Hello,
I would also prefer to make this change after the release of Airflow 2.0.
I would like to suggest that we use black without normalizing strings. In
my opinion, using two apostrophes in one file style does not affect the
readability of the code. On other hand, this can drastically reduce the
number of changes that we will have to make in the code.
-S, --skip-string-normalization
Don't normalize string quotes or prefixes.
Best regards,
Kamil
On Sun, Jun 28, 2020 at 11:50 PM Jarek Potiuk <jarek.pot...@polidea.com>
wrote:
> Unfortunately it won't work this way (at least not by default) - the
> problem is that from what I could see (it's not very clear in the
> documentation), the 'pre-commit' hooks are only run on rebase or
> cherry-pick if there is a conflict - and only AFTER the conflict is
> resolved ( and --continue is used). In our case we want to apply black
> before conflict check.
>
> I looked for some switch to enable it (my previous experience is that if I
> want to do something with git it's just a matter of finding the right
> switch) but could not find any easy way to do it.
>
> In the recent pre-commit you can install pre-commit for "pre-commit-merge"
> hook (run for merges) , but not "pre-applypatch" hook (run for patch am) -
> but from what I looked at, neither of them is called before 'cherry-pick'
> or 'rebase' is done.
>
> Basically we would need to run pre-commit black BEFORE the conflict-check -
> so before every single commit from the rebase is applied. Which I think is
> not currently possible with git (but I'd love to be mistaken on that -
> maybe there is an option/flag/approach in git that I could not find :) ).
>
> J.
>
>
> On Sun, Jun 28, 2020 at 5:05 PM Kaxil Naik <kaxiln...@gmail.com> wrote:
>
> > Do we need a script though? They would only need to rebase their PR on
> > master and the pre-commit hook will update their code with "black" too
> >
> > On Sun, Jun 28, 2020 at 3:53 PM Jarek Potiuk <jarek.pot...@polidea.com>
> > wrote:
> >
> > > And one more thing - such script/tool might be a nice tool to give for
> > > people doing rebases of their PRs from pre-black to post-black. This
> way
> > we
> > > will make it easier for people to rebase their changes after black. I
> > think
> > > it should simply take all the commits that we are reading and apply
> > > black formatting to every commit being rebased. This should make an
> easy
> > > path for people to switch. We can prepare such a script and test it
> now -
> > > rebasing some of the PRs in progress just to test it - and even test
> some
> > > cherry-picking after that. I am happy to help with making the tool and
> > > testing it.
> > >
> > > J.
> > >
> > > On Sun, Jun 28, 2020 at 12:08 PM Jarek Potiuk <
> jarek.pot...@polidea.com>
> > > wrote:
> > >
> > > > Agree with Black and agree with Ash and Tomek about cherry-pickiness.
> > ..
> > > > But I also think this is the right time to do it.
> > > >
> > > > After 1.10.11 we will have some cherry-picks to do for 1.10.12
> > > > undoubtedly. However I presume we will be much closer to releasing
> 2.0
> > > and
> > > > we will not cherry-pick any of the past changes (let's make sure in
> > > > 1.10.11rc1 we will cherry-pick everything that we have to from the
> > > master).
> > > > Also the 1.10.12 changes will be more of bug-fixes rather than
> bringing
> > > any
> > > > of the old features in.
> > > >
> > > > Then I think it is crucial to run Black formatting at the same time
> in
> > > > v1-10-test/v1-10-stable and in master (right after we release 1.10.11
> > > rc1).
> > > > This way any "fixes" that should be cherry-picked to 1.10.11rc2 etc
> > will
> > > > use black for both - master and 1.10.11.
> > > >
> > > > If we do it at the same time, then cherry-picking all the future
> > changes
> > > > should not be a problem. I think the problem with cherry-picking will
> > > only
> > > > happen if we try to cherry-pick pre-black change to post-black code.
> > And
> > > > even then I think we can do some clever automation. I am sure we can
> > > easily
> > > > write a simple script to extract the commit to cherry-pick, apply
> black
> > > and
> > > > only then apply the cherry-picked commit. That should be rather easy
> to
> > > do
> > > > - and it will enable us to cherry-pick both pre-black and post-black
> > > > commits.
> > > >
> > > > I am willing to spend more time on cherry-picking in 1.10.12 - I
> think
> > > > benefits of having an uncompromising code formatter are much more
> "time
> > > for
> > > > review" saving than "time for cherry-picking" adding.
> > > >
> > > > J,
> > > >
> > > > On Sun, Jun 28, 2020 at 11:10 AM Tomasz Urbaszek <
> turbas...@apache.org
> > >
> > > > wrote:
> > > >
> > > >> This is a good idea, and for new code in Airflow I usually use
> black.
> > > >> However, I agree with Ash that this can make cherrypicking harder
> even
> > > >> if we run black on v1-10-test branch (there are already differences
> in
> > > >> code). Personally I would be in favor of introducing black and other
> > > >> things (i.e. pyupgrade) when releasing 2.0.
> > > >>
> > > >> Regarding the issue of merge conflict in PR - we will not be able to
> > > >> avoid it (however running black locally before rebase should help I
> > > >> think).
> > > >>
> > > >> Cheers,
> > > >> Tomek
> > > >>
> > > >>
> > > >> On Sun, Jun 28, 2020 at 12:51 AM Ash Berlin-Taylor <a...@apache.org>
> > > >> wrote:
> > > >> >
> > > >> > Yes, to Black in principle. But (and this is a big but) no, not
> yet.
> > > Or
> > > >> not without testing how it affects our ability to cherry pick back
> to
> > > the
> > > >> release branch.
> > > >> >
> > > >> > (My default would be to assume it makes them harder/almost
> > impossible
> > > >> and this should be the almost last thing we do before we release
> 2.0)
> > > >> >
> > > >> > -ash
> > > >> >
> > > >> > On 27 June 2020 22:02:41 BST, Philippe Gagnon <
> > philgagn...@gmail.com>
> > > >> wrote:
> > > >> > >It's a good idea.
> > > >> > >
> > > >> > >It will make reading the codebase easier, and besides the whole
> > > Python
> > > >> > >ecosystem is slowly moving towards adopting this code style. I
> > > >> > >personally
> > > >> > >have been a fan ever since the project launched.
> > > >> > >
> > > >> > >With regards to open PRs requiring a rebase, it's an annoyance
> for
> > > sure
> > > >> > >but
> > > >> > >if we do decide to standardize on any code style (which we
> should,
> > > >> > >black or
> > > >> > >not), we'll have to pull the bandaid eventually.
> > > >> > >
> > > >> > >Just my two cents. ;-)
> > > >> > >
> > > >> > >Philippe
> > > >> > >
> > > >> > >On Sat, Jun 27, 2020 at 4:13 PM Kaxil Naik <kaxiln...@gmail.com>
> > > >> wrote:
> > > >> > >
> > > >> > >> Hi all,
> > > >> > >>
> > > >> > >> I would like to open the discussion to enable Black (
> > > >> > >> https://github.com/psf/black) - *The Uncompromising Code
> > > Formatter*
> > > >> > >for
> > > >> > >> automatic formatting of Airflow's entire codebase.
> > > >> > >>
> > > >> > >> I have created a WIP PR at
> > > >> > >https://github.com/apache/airflow/pull/9550
> > > >> > >>
> > > >> > >> Some of the caveats:
> > > >> > >>
> > > >> > >> - All the currently open PRs would have some kind of
> conflict
> > > >> > >errors
> > > >> > >> - It *might *make backporting harder (but should be ok'ish
> if
> > we
> > > >> > >enable
> > > >> > >> black on v1-10-test but not 100%)
> > > >> > >> - There are known issues with "line-lengths" not being
> > honoured
> > > by
> > > >> > >black
> > > >> > >> (https://github.com/psf/black/issues/1161) and the
> workaround
> > > is
> > > >> > >to use
> > > >> > >> "#fmt: off".
> > > >> > >>
> > > >> > >> I would love to hear what the community thinks about it.
> > > >> > >>
> > > >> > >> Regards,
> > > >> > >> Kaxil
> > > >> > >>
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >>
> > > >> Tomasz Urbaszek
> > > >> Polidea | Software Engineer
> > > >>
> > > >> M: +48 505 628 493
> > > >> E: tomasz.urbas...@polidea.com
> > > >>
> > > >> Unique Tech
> > > >> Check out our projects!
> > > >>
> > > >
> > > >
> > > > --
> > > >
> > > > Jarek Potiuk
> > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > >
> > > > M: +48 660 796 129 <+48660796129>
> > > > [image: Polidea] <https://www.polidea.com/>
> > > >
> > > >
> > >
> > > --
> > >
> > > Jarek Potiuk
> > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > >
> > > M: +48 660 796 129 <+48660796129>
> > > [image: Polidea] <https://www.polidea.com/>
> > >
> >
>
>
> --
>
> Jarek Potiuk
> Polidea <https://www.polidea.com/> | Principal Software Engineer
>
> M: +48 660 796 129 <+48660796129>
> [image: Polidea] <https://www.polidea.com/>
>
