airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jarek Potiuk <Jarek.Pot...@polidea.com>
Subject Re: [DISCUSS] Enable 'Black' for Auto Code Formatting
Date Sun, 28 Jun 2020 21:50:23 GMT
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
> > >> 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/>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message