You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airflow.apache.org by Kaxil Naik <ka...@gmail.com> on 2020/06/27 20:13:12 UTC
[DISCUSS] Enable 'Black' for Auto Code Formatting
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
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Jarek Potiuk <Ja...@polidea.com>.
Cool. I will try to make some tests with several open PRs how rebasing
works with and I think if we find it easy, I am all for merging it asap.
On Sun, Jul 12, 2020 at 2:01 AM Kaxil Naik <ka...@gmail.com> wrote:
> Updated the PR and added "skip-string-normalization = true" flag
>
> On Fri, Jul 10, 2020 at 8:36 AM Kaxil Naik <ka...@gmail.com> wrote:
>
> > Awesome, thanks Jakob for the suggestion. Will update PR and we will keep
> > that in mind when rolling out the change :)
> >
> > On Fri, Jul 10, 2020 at 5:58 AM Jarek Potiuk <Ja...@polidea.com>
> > wrote:
> >
> >> > 3. Everyone with open PRs rebases with git checkout --theirs -- which
> >> keeps
> >> > their pending changes
> >> >
> >>
> >> I KNEW there must be the right git flag for that :). Happy to try this
> :)
> >>
> >>
> >> > 4. Everyone runs the autoformat (they won't go green until they do)
> >> >
> >> > Reference on checkout --theirs
> >> >
> >>
> https://howchoo.com/g/njcyntcxmwq/git-merge-conflicts-rebase-ours-theirs
> >> >
> >> > On Wed, Jul 1, 2020 at 6:10 PM Daniel Imberman <
> >> daniel.imberman@gmail.com>
> >> > wrote:
> >> >
> >> > > I think that once we add integration tests (which I’ll do now that
> we
> >> > have
> >> > > helm for k8s) and we no longer have to back port this is a great
> idea.
> >> > >
> >> > > via Newton Mail [
> >> > >
> >> >
> >>
> https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.50&pv=10.14.6&source=email_footer_2
> >> > > ]
> >> > > On Wed, Jul 1, 2020 at 4:17 PM, Kaxil Naik <ka...@gmail.com>
> >> wrote:
> >> > > That sounds good. I will turn off String Normalization.
> >> > >
> >> > > On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <
> dpstandish@gmail.com
> >> >
> >> > > wrote:
> >> > >
> >> > > > +1 no string normalization
> >> > > >
> >> > > > On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <
> >> > kamil.bregula@polidea.com>
> >> > > > wrote:
> >> > > >
> >> > > > > 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.Potiuk@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 <
> kaxilnaik@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.Potiuk@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.Potiuk@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 <
> >> > > > > > turbaszek@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 <
> >> > > > > ash@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 <
> >> > > > > > > philgagnon1@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 <
> >> > > > > kaxilnaik@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 <+48%20505%20628%20493>
> >> > > > > > > > >> E: tomasz.urbaszek@polidea.com
> >> > > > > > > > >>
> >> > > > > > > > >> Unique Tech
> >> > > > > > > > >> Check out our projects!
> >> > > > > > > > >>
> >> > > > > > > > >
> >> > > > > > > > >
> >> > > > > > > > > --
> >> > > > > > > > >
> >> > > > > > > > > Jarek Potiuk
> >> > > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> >> > > Engineer
> >> > > > > > > > >
> >> > > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> >> > > <+48%20660%20796%20129>>
> >> > > > > > > > > [image: Polidea] <https://www.polidea.com/>
> >> > > > > > > > >
> >> > > > > > > > >
> >> > > > > > > >
> >> > > > > > > > --
> >> > > > > > > >
> >> > > > > > > > Jarek Potiuk
> >> > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> >> > Engineer
> >> > > > > > > >
> >> > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> >> > > <+48%20660%20796%20129>>
> >> > > > > > > > [image: Polidea] <https://www.polidea.com/>
> >> > > > > > > >
> >> > > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > > --
> >> > > > > >
> >> > > > > > Jarek Potiuk
> >> > > > > > Polidea <https://www.polidea.com/> | Principal Software
> >> Engineer
> >> > > > > >
> >> > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> >> > > <+48%20660%20796%20129>>
> >> > > > > > [image: Polidea] <https://www.polidea.com/>
> >> > > > > >
> >> > > > >
> >> > > >
> >> >
> >> >
> >> >
> >> > --
> >> >
> >> > *Jacob Ferriero*
> >> >
> >> > Strategic Cloud Engineer: Data Engineering
> >> >
> >> > jferriero@google.com
> >> >
> >> > 617-714-2509
> >> >
> >>
> >>
> >> --
> >>
> >> 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/>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Kaxil Naik <ka...@gmail.com>.
Updated the PR and added "skip-string-normalization = true" flag
On Fri, Jul 10, 2020 at 8:36 AM Kaxil Naik <ka...@gmail.com> wrote:
> Awesome, thanks Jakob for the suggestion. Will update PR and we will keep
> that in mind when rolling out the change :)
>
> On Fri, Jul 10, 2020 at 5:58 AM Jarek Potiuk <Ja...@polidea.com>
> wrote:
>
>> > 3. Everyone with open PRs rebases with git checkout --theirs -- which
>> keeps
>> > their pending changes
>> >
>>
>> I KNEW there must be the right git flag for that :). Happy to try this :)
>>
>>
>> > 4. Everyone runs the autoformat (they won't go green until they do)
>> >
>> > Reference on checkout --theirs
>> >
>> https://howchoo.com/g/njcyntcxmwq/git-merge-conflicts-rebase-ours-theirs
>> >
>> > On Wed, Jul 1, 2020 at 6:10 PM Daniel Imberman <
>> daniel.imberman@gmail.com>
>> > wrote:
>> >
>> > > I think that once we add integration tests (which I’ll do now that we
>> > have
>> > > helm for k8s) and we no longer have to back port this is a great idea.
>> > >
>> > > via Newton Mail [
>> > >
>> >
>> https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.50&pv=10.14.6&source=email_footer_2
>> > > ]
>> > > On Wed, Jul 1, 2020 at 4:17 PM, Kaxil Naik <ka...@gmail.com>
>> wrote:
>> > > That sounds good. I will turn off String Normalization.
>> > >
>> > > On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <dpstandish@gmail.com
>> >
>> > > wrote:
>> > >
>> > > > +1 no string normalization
>> > > >
>> > > > On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <
>> > kamil.bregula@polidea.com>
>> > > > wrote:
>> > > >
>> > > > > 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.Potiuk@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 <kaxilnaik@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.Potiuk@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.Potiuk@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 <
>> > > > > > turbaszek@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 <
>> > > > > ash@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 <
>> > > > > > > philgagnon1@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 <
>> > > > > kaxilnaik@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 <+48%20505%20628%20493>
>> > > > > > > > >> E: tomasz.urbaszek@polidea.com
>> > > > > > > > >>
>> > > > > > > > >> Unique Tech
>> > > > > > > > >> Check out our projects!
>> > > > > > > > >>
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > --
>> > > > > > > > >
>> > > > > > > > > Jarek Potiuk
>> > > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
>> > > Engineer
>> > > > > > > > >
>> > > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
>> > > <+48%20660%20796%20129>>
>> > > > > > > > > [image: Polidea] <https://www.polidea.com/>
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > > > --
>> > > > > > > >
>> > > > > > > > Jarek Potiuk
>> > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
>> > Engineer
>> > > > > > > >
>> > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
>> > > <+48%20660%20796%20129>>
>> > > > > > > > [image: Polidea] <https://www.polidea.com/>
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > > >
>> > > > > > --
>> > > > > >
>> > > > > > Jarek Potiuk
>> > > > > > Polidea <https://www.polidea.com/> | Principal Software
>> Engineer
>> > > > > >
>> > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
>> > > <+48%20660%20796%20129>>
>> > > > > > [image: Polidea] <https://www.polidea.com/>
>> > > > > >
>> > > > >
>> > > >
>> >
>> >
>> >
>> > --
>> >
>> > *Jacob Ferriero*
>> >
>> > Strategic Cloud Engineer: Data Engineering
>> >
>> > jferriero@google.com
>> >
>> > 617-714-2509
>> >
>>
>>
>> --
>>
>> Jarek Potiuk
>> Polidea <https://www.polidea.com/> | Principal Software Engineer
>>
>> M: +48 660 796 129 <+48660796129>
>> [image: Polidea] <https://www.polidea.com/>
>>
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Kaxil Naik <ka...@gmail.com>.
Awesome, thanks Jakob for the suggestion. Will update PR and we will keep
that in mind when rolling out the change :)
On Fri, Jul 10, 2020 at 5:58 AM Jarek Potiuk <Ja...@polidea.com>
wrote:
> > 3. Everyone with open PRs rebases with git checkout --theirs -- which
> keeps
> > their pending changes
> >
>
> I KNEW there must be the right git flag for that :). Happy to try this :)
>
>
> > 4. Everyone runs the autoformat (they won't go green until they do)
> >
> > Reference on checkout --theirs
> > https://howchoo.com/g/njcyntcxmwq/git-merge-conflicts-rebase-ours-theirs
> >
> > On Wed, Jul 1, 2020 at 6:10 PM Daniel Imberman <
> daniel.imberman@gmail.com>
> > wrote:
> >
> > > I think that once we add integration tests (which I’ll do now that we
> > have
> > > helm for k8s) and we no longer have to back port this is a great idea.
> > >
> > > via Newton Mail [
> > >
> >
> https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.50&pv=10.14.6&source=email_footer_2
> > > ]
> > > On Wed, Jul 1, 2020 at 4:17 PM, Kaxil Naik <ka...@gmail.com>
> wrote:
> > > That sounds good. I will turn off String Normalization.
> > >
> > > On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <dp...@gmail.com>
> > > wrote:
> > >
> > > > +1 no string normalization
> > > >
> > > > On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <
> > kamil.bregula@polidea.com>
> > > > wrote:
> > > >
> > > > > 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.Potiuk@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 <ka...@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.Potiuk@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.Potiuk@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 <
> > > > > > turbaszek@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 <
> > > > > ash@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 <
> > > > > > > philgagnon1@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 <
> > > > > kaxilnaik@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 <+48%20505%20628%20493>
> > > > > > > > >> E: tomasz.urbaszek@polidea.com
> > > > > > > > >>
> > > > > > > > >> Unique Tech
> > > > > > > > >> Check out our projects!
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > >
> > > > > > > > > Jarek Potiuk
> > > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> > > Engineer
> > > > > > > > >
> > > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> > > <+48%20660%20796%20129>>
> > > > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > >
> > > > > > > > Jarek Potiuk
> > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> > Engineer
> > > > > > > >
> > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> > > <+48%20660%20796%20129>>
> > > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > >
> > > > > > Jarek Potiuk
> > > > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > > > >
> > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> > > <+48%20660%20796%20129>>
> > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > >
> > > > >
> > > >
> >
> >
> >
> > --
> >
> > *Jacob Ferriero*
> >
> > Strategic Cloud Engineer: Data Engineering
> >
> > jferriero@google.com
> >
> > 617-714-2509
> >
>
>
> --
>
> Jarek Potiuk
> Polidea <https://www.polidea.com/> | Principal Software Engineer
>
> M: +48 660 796 129 <+48660796129>
> [image: Polidea] <https://www.polidea.com/>
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Jarek Potiuk <Ja...@polidea.com>.
> 3. Everyone with open PRs rebases with git checkout --theirs -- which keeps
> their pending changes
>
I KNEW there must be the right git flag for that :). Happy to try this :)
> 4. Everyone runs the autoformat (they won't go green until they do)
>
> Reference on checkout --theirs
> https://howchoo.com/g/njcyntcxmwq/git-merge-conflicts-rebase-ours-theirs
>
> On Wed, Jul 1, 2020 at 6:10 PM Daniel Imberman <da...@gmail.com>
> wrote:
>
> > I think that once we add integration tests (which I’ll do now that we
> have
> > helm for k8s) and we no longer have to back port this is a great idea.
> >
> > via Newton Mail [
> >
> https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.50&pv=10.14.6&source=email_footer_2
> > ]
> > On Wed, Jul 1, 2020 at 4:17 PM, Kaxil Naik <ka...@gmail.com> wrote:
> > That sounds good. I will turn off String Normalization.
> >
> > On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <dp...@gmail.com>
> > wrote:
> >
> > > +1 no string normalization
> > >
> > > On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <
> kamil.bregula@polidea.com>
> > > wrote:
> > >
> > > > 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.Potiuk@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 <ka...@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.Potiuk@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.Potiuk@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 <
> > > > > turbaszek@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 <
> > > > ash@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 <
> > > > > > philgagnon1@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 <
> > > > kaxilnaik@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 <+48%20505%20628%20493>
> > > > > > > >> E: tomasz.urbaszek@polidea.com
> > > > > > > >>
> > > > > > > >> Unique Tech
> > > > > > > >> Check out our projects!
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > >
> > > > > > > > Jarek Potiuk
> > > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> > Engineer
> > > > > > > >
> > > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> > <+48%20660%20796%20129>>
> > > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > >
> > > > > > > Jarek Potiuk
> > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> Engineer
> > > > > > >
> > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> > <+48%20660%20796%20129>>
> > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > >
> > > > > Jarek Potiuk
> > > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > > >
> > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> > <+48%20660%20796%20129>>
> > > > > [image: Polidea] <https://www.polidea.com/>
> > > > >
> > > >
> > >
>
>
>
> --
>
> *Jacob Ferriero*
>
> Strategic Cloud Engineer: Data Engineering
>
> jferriero@google.com
>
> 617-714-2509
>
--
Jarek Potiuk
Polidea <https://www.polidea.com/> | Principal Software Engineer
M: +48 660 796 129 <+48660796129>
[image: Polidea] <https://www.polidea.com/>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Jacob Ferriero <jf...@google.com.INVALID>.
Regarding "can we do this with the right git flags":
When Apache Beam community did something similar to standardize on
"Spotless Java Everywhere" [BEAM-4394] they took the following approach
<https://issues.apache.org/jira/browse/BEAM-4394?focusedCommentId=16495811&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16495811>
Is there a reason this rebasing via "git checkout --theirs -- and re-run
black to format pending changes" approach would not work with black?
Note this is likely orthogonal to the concern about complicating backport /
cherrypicking but might smooth the process for open PRs.
Quote from relevant BEAM JIRA Comment:
The impact on contributors is almost nothing with good communication and
timing.
1. Tell everyone that it is happening
2. Turn on enforcement [and run formatter over whole repo]
3. Everyone with open PRs rebases with git checkout --theirs -- which keeps
their pending changes
4. Everyone runs the autoformat (they won't go green until they do)
Reference on checkout --theirs
https://howchoo.com/g/njcyntcxmwq/git-merge-conflicts-rebase-ours-theirs
On Wed, Jul 1, 2020 at 6:10 PM Daniel Imberman <da...@gmail.com>
wrote:
> I think that once we add integration tests (which I’ll do now that we have
> helm for k8s) and we no longer have to back port this is a great idea.
>
> via Newton Mail [
> https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.50&pv=10.14.6&source=email_footer_2
> ]
> On Wed, Jul 1, 2020 at 4:17 PM, Kaxil Naik <ka...@gmail.com> wrote:
> That sounds good. I will turn off String Normalization.
>
> On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <dp...@gmail.com>
> wrote:
>
> > +1 no string normalization
> >
> > On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <ka...@polidea.com>
> > wrote:
> >
> > > 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.Potiuk@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 <ka...@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.Potiuk@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.Potiuk@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 <
> > > > turbaszek@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 <
> > > ash@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 <
> > > > > philgagnon1@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 <
> > > kaxilnaik@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 <+48%20505%20628%20493>
> > > > > > >> E: tomasz.urbaszek@polidea.com
> > > > > > >>
> > > > > > >> Unique Tech
> > > > > > >> Check out our projects!
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > >
> > > > > > > Jarek Potiuk
> > > > > > > Polidea <https://www.polidea.com/> | Principal Software
> Engineer
> > > > > > >
> > > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> <+48%20660%20796%20129>>
> > > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > --
> > > > > >
> > > > > > Jarek Potiuk
> > > > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > > > >
> > > > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> <+48%20660%20796%20129>>
> > > > > > [image: Polidea] <https://www.polidea.com/>
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > >
> > > > Jarek Potiuk
> > > > Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > >
> > > > M: +48 660 796 129 <+48%20660%20796%20129> <+48660796129
> <+48%20660%20796%20129>>
> > > > [image: Polidea] <https://www.polidea.com/>
> > > >
> > >
> >
--
*Jacob Ferriero*
Strategic Cloud Engineer: Data Engineering
jferriero@google.com
617-714-2509
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Daniel Imberman <da...@gmail.com>.
I think that once we add integration tests (which I’ll do now that we have helm for k8s) and we no longer have to back port this is a great idea.
via Newton Mail [https://cloudmagic.com/k/d/mailapp?ct=dx&cv=10.0.50&pv=10.14.6&source=email_footer_2]
On Wed, Jul 1, 2020 at 4:17 PM, Kaxil Naik <ka...@gmail.com> wrote:
That sounds good. I will turn off String Normalization.
On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <dp...@gmail.com>
wrote:
> +1 no string normalization
>
> On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <ka...@polidea.com>
> wrote:
>
> > 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 <Ja...@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 <ka...@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.Potiuk@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.Potiuk@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 <
> > > turbaszek@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 <
> > ash@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 <
> > > > philgagnon1@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 <
> > kaxilnaik@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.urbaszek@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/>
> > >
> >
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Kaxil Naik <ka...@gmail.com>.
That sounds good. I will turn off String Normalization.
On Thu, Jul 2, 2020 at 12:11 AM Daniel Standish <dp...@gmail.com>
wrote:
> +1 no string normalization
>
> On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <ka...@polidea.com>
> wrote:
>
> > 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 <Ja...@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 <ka...@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.Potiuk@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.Potiuk@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 <
> > > turbaszek@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 <
> > ash@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 <
> > > > philgagnon1@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 <
> > kaxilnaik@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.urbaszek@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/>
> > >
> >
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Daniel Standish <dp...@gmail.com>.
+1 no string normalization
On Wed, Jul 1, 2020 at 3:20 PM Kamil Breguła <ka...@polidea.com>
wrote:
> 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 <Ja...@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 <ka...@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.Potiuk@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.Potiuk@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 <
> > turbaszek@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 <
> ash@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 <
> > > philgagnon1@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 <
> kaxilnaik@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.urbaszek@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/>
> >
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Kamil Breguła <ka...@polidea.com>.
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 <Ja...@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 <ka...@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 <Ja...@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.Potiuk@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 <
> turbaszek@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 <as...@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 <
> > philgagnon1@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 <ka...@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.urbaszek@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/>
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Jarek Potiuk <Ja...@polidea.com>.
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 <ka...@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 <Ja...@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 <Ja...@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 <turbaszek@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 <as...@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 <
> philgagnon1@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 <ka...@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.urbaszek@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/>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Kaxil Naik <ka...@gmail.com>.
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 <Ja...@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 <Ja...@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 <tu...@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 <as...@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 <ph...@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 <ka...@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.urbaszek@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/>
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Jarek Potiuk <Ja...@polidea.com>.
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 <Ja...@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 <tu...@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 <as...@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 <ph...@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 <ka...@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.urbaszek@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/>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Jarek Potiuk <Ja...@polidea.com>.
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 <tu...@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 <as...@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 <ph...@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 <ka...@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.urbaszek@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/>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Tomasz Urbaszek <tu...@apache.org>.
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 <as...@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 <ph...@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 <ka...@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.urbaszek@polidea.com
Unique Tech
Check out our projects!
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Kaxil Naik <ka...@gmail.com>.
Yup, I have a Draft PR already, once the Scheduler HA and the Task Instance
Endpoints PRs are merged, I will rebase that PR.
On Wed, Oct 7, 2020 at 8:00 PM Tomasz Urbaszek <tu...@apache.org> wrote:
> As we are close to releasing 2.0 alpha it seems feasible to resurrect this
> thread.
>
> I think we should run black on 2.0 as the last commit before releasing it.
> I would even argue that it should be done for alpha release.
>
> What do others think?
>
> Bests,
> Tomek
>
> On 2020/06/27 22:51:04, Ash Berlin-Taylor <as...@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 <ph...@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 <ka...@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
> > >>
> >
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Tomasz Urbaszek <tu...@apache.org>.
As we are close to releasing 2.0 alpha it seems feasible to resurrect this thread.
I think we should run black on 2.0 as the last commit before releasing it. I would even argue that it should be done for alpha release.
What do others think?
Bests,
Tomek
On 2020/06/27 22:51:04, Ash Berlin-Taylor <as...@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 <ph...@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 <ka...@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
> >>
>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Ash Berlin-Taylor <as...@apache.org>.
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 <ph...@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 <ka...@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
>>
Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Posted by Philippe Gagnon <ph...@gmail.com>.
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 <ka...@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
>