Changes are fine (Other than adding the annotation to empty __init__
files), but having this as 47 PRs I think is unhelpful when it comes to
spelunking changes in Git History (i.e. using `git blame`, which I at
least seem to do once every couple of weeks) so I would prefer it there
was 1 for core airflow, and one for providers, say.
I don't feel that strongly about this, it's just how I would have done
it personally -- and having a single rev lets me more easily ignore
that rev.
-ash
On Sun, Sep 11 2022 at 14:29:56 +01:00:00, Ash Berlin-Taylor
<a...@apache.org> wrote:
Right okay, gotcha. I'll take a look tomorrow if no one does before
hand.
On 11 September 2022 13:33:27 BST, Jarek Potiuk <pot...@apache.org>
wrote:
Yeah. It was a bit late. But this was the main reasoning - if we
merge them quickly AND cherry-pick those to 2.4 branch we have a
very good chance to avoid any accidental merge of something that was
merged since the 2.4 branch cut-off - there were literally a handful
of those PRs merged and it will be immediately visible (this is also
one reason why I split it into many small PRs to detect and fix any
kind of problems and remove just merged code while cherry-picking).
This was the reasoning behind doing it fast when I recalled that we
hold off from it in the middle of 2.3 development.
J,.
On Sun, Sep 11, 2022 at 1:19 PM Ash Berlin-Taylor <a...@apache.org
<mailto:a...@apache.org>> wrote:
One comment on the timing: but doing it now you make cherry picks
for 2.4.x harder, of which were about to do a lot of. The best time
to do such a change is just _before_ cutting the next main branch.
On 10 September 2022 21:38:12 BST, Jarek Potiuk <pot...@apache.org
<mailto: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 <mailto: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
