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 10:08:27 GMT
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/>

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