Updated the PR and added "skip-string-normalization = true" flag On Fri, Jul 10, 2020 at 8:36 AM Kaxil Naik <kaxiln...@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 <jarek.pot...@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.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 >> > >> >> >> -- >> >> Jarek Potiuk >> Polidea <https://www.polidea.com/> | Principal Software Engineer >> >> M: +48 660 796 129 <+48660796129> >> [image: Polidea] <https://www.polidea.com/> >> >
