s
>> > > this
>> > > > > one
>> > > > > > > > more comment not addressed".
>> > > > > > > >
>> > > > > > > > On Wed, Dec 20, 2023 at 5:16 AM Amogh Desai <
>> > > > > amoghdesai
where we try to
> > > resolve
> > > > > all
> > > > > > > the
> > > > > > > >> comments from the maintainers
> > > > > > > >> before performing a merge. But what we also do is that if a
>
gt; > > > >>
> > > > > > >>
> > > > > > >> Thanks & Regards,
> > > > > > >> Amogh Desai
> > > > > > >>
> > > > > > >> On Wed, Dec 20, 2023 at 5:11 AM Jarek Potiuk <
hat the person who
> > > left
> > > > > the
> > > > > >> > comment has to resolve it, then I'm alright with trying
> this. I
> > > > don't
> > > > > >> want
> > > > > >> > a PR to get bloc
; > > just
> > > > a
> > > > >> > "comment" not an invitation to conversation. Or when you are an
> > > > author,
> > > > >> and
> > > > >> > you see "All right - I am really done with it, now
gt; all
> > > >> > the remaining conversations".
> > > >> >
> > > >> > Note that not every "comment" is one that gets into "resolvable"
> > > >> > conversation. There are generic comments for the whole PR that
) of code are "conversations" - when they
> > >> relate
> > >> > to a particular line of code. And those tend to be actual issues,
> > >> questions
> > >> > or doubts that **should** get some reaction IMHO.
> > >> >
&g
; >> >
> >> > > Interesting. I generally try to follow that policy as a best
> >> practice
> >> > on
> >> > > my own PRs just so I make sure I didn't miss comments, but there are
> >> als
est
>> practice
>> > on
>> > > my own PRs just so I make sure I didn't miss comments, but there are
>> also
>> > > times I intentionally leave certain discussions "out in the open". I
>> > > guess as long as we are not dict
ng as we are not dictating that the person who left the
> > comment
> > > has to resolve it, then I'm alright with trying this. I don't want a
> PR
> > to
> > > get blocked because of a drive-by comment.
> > >
> > > Seems like
t with trying this. I don't want a PR
> to
> > get blocked because of a drive-by comment.
> >
> > Seems like this is easily reversible and we can give it a trial run and
> > decide later if we want to keep it.
> >
> >
> > - ferruzzi
> >
> >
> >
rek Potiuk
> Sent: Tuesday, December 19, 2023 12:45 PM
> To: dev@airflow.apache.org
> Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [DISCUSS] "Require conversation
> resolution" in our PRs before merge?
>
> CAUTION: This email originated from outside of the organization. Do not
>
__
From: Jarek Potiuk
Sent: Tuesday, December 19, 2023 12:45 PM
To: dev@airflow.apache.org
Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [DISCUSS] "Require conversation
resolution" in our PRs before merge?
CAUTION: This email originated from outside of the organization. Do not click
l
> Given the slow down over the holidays I don't think two weeks will be
enough - make it 4?
Ah. True :)
On Tue, Dec 19, 2023 at 9:41 PM Ash Berlin-Taylor wrote:
> Given the slow down over the holidays I don't think two weeks will be
> enough - make it 4?
>
> On 19 December 2023 20:33:23 GMT, Ja
Given the slow down over the holidays I don't think two weeks will be enough -
make it 4?
On 19 December 2023 20:33:23 GMT, Jarek Potiuk wrote:
>Ash:
>
>> I.e. Convention over enforcement and treating people as mature adults not
>children who need guard rails.
>
>I think it's quite the opposite,
Ash:
> I.e. Convention over enforcement and treating people as mature adults not
children who need guard rails.
I think it's quite the opposite, Both 1) 2) and 3) reasoning is more of the
aid for whoever looks at the PR that there are still some conversations not
addressed, I personally feel it's
Answering some of the recent questions.
Daniel:
> E.g. when you "resolve" a conversation, then you make it less visible.
This isn't always a good thing. Sometimes you just want to +1 it. When
others visit the PR, then they will not see the conversation. Maybe they
would want to engage in discu
This reflect my feelings as well. I'm not convinced we are solving something
that needs to be solved.
B.
Sent from my iPhone
> On 19 Dec 2023, at 21:05, Ash Berlin-Taylor wrote:
>
> Weak -1 from me, because I don't think this needs to be enforced/required
> part of the workflow.
>
> I.e. C
Weak -1 from me, because I don't think this needs to be enforced/required part
of the workflow.
I.e. Convention over enforcement and treating people as mature adults not
children who need guard rails.
-ash
On 19 December 2023 13:12:05 GMT, Jarek Potiuk wrote:
>Hey everyone,
>
>TL;DR; I have a
The proposed rule isn't a bad idea, especially for ensuring that
maintainers wanting to merge have reviewed all conversions. However, it's
essential to permit them to close open conversations if they find the
comments have been addressed. Only ping the commenter if uncertain, with a
maximum waiting
Well let me add some more thoughts.
I like the idea in general, the principle of trying to somehow acknowledge
the comments and suggestions that have been made.
But it may have some unintended and perhaps undesirable consequences.
E.g. when you "resolve" a conversation, then you make it less vis
+1
On Tue, Dec 19, 2023 at 9:36 AM Pierre Jeambrun
wrote:
> This is something I already try to apply on my own PRs, never merge before
> explicitly solving all conversations.
>
> Also for a reviewer, I feel like this gives more confidence to the fact
> that the PR is ready, and indeed we are les
This is something I already try to apply on my own PRs, never merge before
explicitly solving all conversations.
Also for a reviewer, I feel like this gives more confidence to the fact
that the PR is ready, and indeed we are less subject to missing a
discussion or something going on making it 'not
fan Grosch, Dr. Markus Heyn, Dr. Tanja Rückert
-Original Message-
From: Jarek Potiuk
Sent: Dienstag, 19. Dezember 2023 16:58
To: dev@airflow.apache.org
Subject: Re: [DISCUSS] "Require conversation resolution" in our PRs before
merge?
Great questions. More context then, since
Great questions. More context then, since you asked:
The main benefits I see:
1) the person merging the PR will not merge PRs when conversations are
still unresolved manually. Some of the PRS have long conversations and
I had many times - for example - cases where I missed that someone
made a com
I am wondering too if this is not something that gives more work to maintainer
without real benefits. A maintainer can still mark all conversations as
resolved and merge the PR if he wants. Though, I understand there is the
intention here as oppose as today where a maintainer can just miss some
I'm less enthusiastic. What problem are we solving with this? If something has
not been addressed it can be done in a follow up or of if it was just part of
the conversation it won't have impact on the code. In addition, the ones that
need to deal with it are the ones merging and those are not n
+1 for trying and observing how it works. My concern is that adding an
additional obstacle might lead to more unfinished PRs. It might be helpful to
give the contributor some guidance on when we can resolve the comments.
Best,
Wei
> On Dec 19, 2023, at 9:28 PM, Andrey Anshin wrote:
>
> We cou
We could try and if found it slows down for some reason we always might
revert it back.
Just one suggestion, sometimes discussion contains some useful information,
e.g. "What the reason of finally decision", "Useful information why it
should works by suggested way, or should not work", which might
I like the idea, at least double checking what the comments were is
a good cause. +1 to incorporating this.
On 2023/12/19 13:12:05 Jarek Potiuk wrote:
> Hey everyone,
>
> TL;DR; I have a small proposal/discussion proposal to modify a bit the
> branch protection rules for Airflow. Why don't we
Hey everyone,
TL;DR; I have a small proposal/discussion proposal to modify a bit the
branch protection rules for Airflow. Why don't we add a protection
rule in our PRs that requires all the comments in the PRs to be
"marked as resolved" before merging the PR ?
I have been following myself - for
31 matches
Mail list logo
