BTW. All the 47 PRs are "green" now https://github.com/apache/airflow/pulls?q=is%3Aopen+is%3Apr+label%3Afuture-annotations and if we want we could just merge them and get __future__. annotations applied automatically.
But I am happy for any direction to either close it or wait with them, or do it completely differently, or even not do it at all. If need be, I've learned everything to re-do all the PRs in an even more automated way any time we decide to do it, so this is not a problem to drop it if needed. Would love to hear what others think. And apologies if that caused any distress (to TP but also anyone who could see it as a problem, I was not aware this might be a problem :). J. On Sat, Sep 10, 2022 at 10:38 PM Jarek Potiuk <pot...@apache.org> wrote: > Hey Everyone, > > TL;DR; I planned to ask for help in reviewing the 48 (pretty much > fully automated PRs I prepared, but a comment from TP made me think that > maybe I went too much ahead and maybe there are other ideas on implementing > the change (or even maybe not implementing it at all). > > More context: > > I have just attempted what I THOUGHT was generally accepted and good idea, > but comment from TP made me think maybe I went ahead of everyone else, so I > am reaching here to see if this was a good idea (And I am ready to back-out > and do it differently - and follow another way if someone can propose it). > > I just (literally while watching Poland <-> Brazil Volleybal World > Championship semi-finals) prepared 48 separate PRs to migrate to > __future__.annotations which (I thought) was considered a good idea and > something we wanted to defer to after v2-4-test branch cut-off because of > the concerns that it might make cherry-picking more complex. It was 9X% > fully automated conversion - by search/replace and adding `from __future__ > import annotations` and letting upgrade do the job (as discussed before). > I also split the one huge PR into 48 (!) much smaller PRs - basically one > per each "airflow" core package, few big providers separately, tests > separately (as they are far less risky than "core code" change). > > We already added __future__ annotation in a number of other places before > - without a "broad" agreement and consensus/voting (different people did in > various places - including myself and others) so I thought this is not > something we need a broader voting/consensus on. > > Again - that was 95% automated change where I contributed mostly the > thinking "how to split '' theI PR and all the rest was literally "git > commit add <package>" (and let pyupgrade and our other pre-commits like > isort and others) do the job. > > It never occured to me that this might be a problem for anyone. My > understanding of the problem was: > * we want to do it > * we were afraid to do it before because it would make cherry-picking to > 2-3 branch more problematic > * we wanted to do it right "when" we cut-off the 2.4 branch (for the same > reason as above - we wanted to avoid cherry-picking problems for 2.4.* > changes) > > So I wanted to make this change as fast as humanly possible - and raise > this multiple PRs so we can review/approve (and I'd cherry-pick them) as > soon as possible to minimise the cherry-picking problem. > > But maybe I was too fast and it's not as straightforward as I thought it > was? (actually it never occured to me that there might be a slightest > problem with it after the earlier discussion). I literally thought about it > as a mechanical change that we want to introduce and that doing that in > small chunks (as discussed in the original PR) was the best approach. The > comment from TP > https://apache-airflow.slack.com/archives/CCPRP7943/p1662837864413849 - > made me think that I have misjudged it. > > I wanted to write that email asking all of the committers here to help to > review and merge all those PRs as fast as possible (again - those are > purely mechanical changes, there are literally a few - literally less than > 5) manual changes I had to make to fix some mypy problems. > All the PRs are here: > https://github.com/apache/airflow/pulls?q=is%3Aopen+is%3Apr+label%3Afuture-annotations > > But then I ask for something else then: > > * do you think this is fine we add __future__ annotations in general > * do you think the way I proposed it is somewhat too In-human (i honestly > feel it is an in-human change - because it is fully automated by our tools > :D). But is it wrong approach to have 48 PRs automatically prepared by our > tools to get a "consistent" approach > * did I do it too fast/too early ? > * any ideas on how we can do it differently to avoid the "cherry-picking" > problems (mentioned by others in the original message)? > > I am really concerned to ask if this is something I have done wrongly or > misunderstood the consensus, and maybe chose the wrong way ? Maybe I should > approach it differently and ask for consensus on such a flurry of PRS? Or > maybe the timing is wrong. > > I am happy to follow in any way there - I can either keep the PRs and > rebase them until the time is good (it takes about two minutes to rebase > every one of them and apply the automation again) if the way I proposed it > was because I misunderstood the "sentiment" about the change - or follow > any other way that will help us to implement the change - or even abandon > it completely if we will agree it's not a good idea. I am also happy to > apply any "general comments" there - now that the PR's are there, applying > any automation even on all of them to improve it, is rather simple. > > Looking forward to any comments. I am really curious how others look at it > - regardless of the outcome - always eager to learn new things :) > > J. > > > > > > > > > > > > > > > On Thu, Jul 7, 2022 at 8:52 AM Tobiasz Kędzierski < > tobiaszkedzier...@gmail.com> wrote: > >> Great idea. +1 from me. Thanks for raising this up Jarek and thanks to >> Pierre for great isort tip :) >> >> BR >> Tobiasz >> >
