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