Dennis: > I guess as long 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.
Yep. I think it is perfectly fine for the person who reviews and wants to merge the code to resolve such obvious cases where a comment was 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 it's time to resolve 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 do not land as "resolvable" conversations. Only the "suggestions" and comments for a particular line (or lines) 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. J. On Tue, Dec 19, 2023 at 11:57 PM Ferruzzi, Dennis <ferru...@amazon.com.invalid> wrote: > 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 also > times I intentionally leave certain discussions "out in the open". I > guess as long 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 this is easily reversible and we can give it a trial run and > decide later if we want to keep it. > > > - ferruzzi > > > ________________________________ > From: Jarek Potiuk <ja...@potiuk.com> > 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 links or open attachments unless you can confirm the sender and know > the content is safe. > > > > AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. > Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne pouvez > pas confirmer l’identité de l’expéditeur et si vous n’êtes pas certain que > le contenu ne présente aucun risque. > > > > > 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 <a...@apache.org> 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, Jarek Potiuk <ja...@potiuk.com> 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, 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 treating people as more adult, when > you > > >allow them to unilaterally say "I believe the conversation is resolved" > > > > > >Bolke: > > > > > >> This reflect my feelings as well. I'm not convinced we are solving > > >something that needs to be solved. > > > > > >I think 1) 2) 3) are real problems that it addresses. > > > > > > > > >If we have no "strong" -1s we can give it a try. It's not a one-way > > street. > > >We can always go back if we see it slows us down or annoys people. > > > > > >We can even set a way how we assess it. Maybe everyone should just > collect > > >cases where it caused some problems - in two weeks or so (should be > > enough). > > >Why not everyone who actively participates in the PR review process > brings > > >their experience and explains if it caused them unnecessary burden for > no > > >gain (also the opposite - where it helped). > > > > > >J. > > > > > > > > >On Tue, Dec 19, 2023 at 9:15 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > >> 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 discussion. And when you get a notification in > > an > > >> email about a comment and want to engage and respond, I've had issues > > with > > >> following links to conversations after resolution before. > > >> > > >> True. However, you still see that there was conversation (and can > always > > >> un-collapse it). Resolving conversation does not "remove" it. Actually > > when > > >> the conversation is resolved. > > >> Also you can see it in the "conversations menu". And well, assumption > is > > >> that resolving conversation makes it well - resolved :). And +1 until > > it is > > >> resolved is still fine and cool (or even after). And as a maintainer, > > you > > >> can always "unresolve" such a conversation. > > >> > > >> [image: image.png] > > >> > > >> Hussein: > > >> > > >> > 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 time (let's say 48 hours on workdays). If the > commenter > > >> doesn't reply and there are no other open conversations, we can merge. > > >> > > >> Absolutely. I think it should be fine to have either the author or the > > >> maintainer to resolve conversations - it all depends on context and > > >> problem. I tend to think about "resolving" conversation as a statement > > of > > >> intention / understanding rather than "certainty". It might be either > > the > > >> author or the maintainer who "BELIEVES" that the conversation is > > resolved. > > >> It's subjective, not objective IMHO. We - as humans - can make > mistakes > > but > > >> as long as we have good intentions, it's fine to resolve conversation > by > > >> either. What matters here is that no conversation is simply "left > > >> unaddressed" and that there was a deliberate action on each > > conversation. > > >> > > >> From > > >> > > > https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations > > >> > > >> > You can resolve a conversation in a pull request if you opened the > > pull > > >> request or if you have write access to the repository where the pull > > >> request was opened. > > >> > > >> So in our case - either the author, or one of the maintainers can mark > > the > > >> conversation as resolved. > > >> > > >> > Could we forbid the authors from closing conversations? > > >> > > >> I am afraid not. > > >> > > >> > > >> On Tue, Dec 19, 2023 at 7:31 PM Hussein Awala <huss...@awala.fr> > wrote: > > >> > > >>> 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 time (let's say 48 hours on workdays). If the > commenter > > >>> doesn't reply and there are no other open conversations, we can > merge. > > >>> Additionally, should we wait for all open conversations or only those > > >>> opened by maintainers? Could we forbid the authors from closing > > >>> conversations? > > >>> > > >>> On Tue 19 Dec 2023 at 19:19, Daniel Standish > > >>> <daniel.stand...@astronomer.io.invalid> wrote: > > >>> > > >>> > +1 > > >>> > > > >>> > On Tue, Dec 19, 2023 at 9:36 AM Pierre Jeambrun < > > pierrejb...@gmail.com> > > >>> > 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 less subject to missing a > > >>> > > discussion or something going on making it 'not ok' to merge. > Going > > >>> over > > >>> > > the entire thread before merging a PR to double check that > > everything > > >>> is > > >>> > > actually addressed can be time consuming. That is especially true > > if > > >>> > things > > >>> > > are not marked as resolved. > > >>> > > > > >>> > > I agree that this is something that adds up some work, but I > think > > it > > >>> is > > >>> > > worth the experiment and see what happens. We can easily revert > if > > we > > >>> are > > >>> > > not happy with the way it goes. > > >>> > > > > >>> > > The workload will most likely be on the contributors' side, that > > will > > >>> > have > > >>> > > to actually address and solve all the conversations. > > >>> > > > > >>> > > Le mar. 19 déc. 2023 à 16:44, Vincent Beck <vincb...@apache.org> > a > > >>> > écrit : > > >>> > > > > >>> > > > 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 comments. I am quite doubtful > but I > > >>> am in > > >>> > > to > > >>> > > > try it out and see how it goes. > > >>> > > > > > >>> > > > On 2023/12/19 14:55:13 Bolke de Bruin wrote: > > >>> > > > > 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 necessarily the same as the ones contributing. > > >>> > > > > > > >>> > > > > So for the friction that it creates with both the committer > and > > >>> the > > >>> > > > contributer what is the benefit? > > >>> > > > > > > >>> > > > > B. > > >>> > > > > > > >>> > > > > > > >>> > > > > Sent from my iPhone > > >>> > > > > > > >>> > > > > > On 19 Dec 2023, at 15:45, Wei Lee <weilee...@gmail.com> > > wrote: > > >>> > > > > > > > >>> > > > > > +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 < > > >>> > > andrey.ans...@taragol.is> > > >>> > > > wrote: > > >>> > > > > >> > > >>> > > > > >> 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 be > > >>> > > > useful > > >>> > > > > >> for someone who investigate why this changes was made, so > in > > >>> this > > >>> > > > case I > > >>> > > > > >> would suggest to create link in the main thread of PR with > > >>> useful > > >>> > > > > >> discussions. > > >>> > > > > >> > > >>> > > > > >> > > >>> > > > > >> > > >>> > > > > >> > > >>> > > > > >>> On Tue, 19 Dec 2023 at 17:16, Jarek Potiuk < > > ja...@potiuk.com> > > >>> > > 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 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 quite some time - an > > >>> approach > > >>> > > > that > > >>> > > > > >>> whenever there are comments/suggestions/doubts in my PRs > I > > do > > >>> not > > >>> > > > > >>> merge the PR until I **think** all of those have been > > >>> addressed > > >>> > > > > >>> (somehow). > > >>> > > > > >>> > > >>> > > > > >>> The resolution might not be what the person commenting > > wants > > >>> > > > directly, > > >>> > > > > >>> it might be "I hear your comment, and there are good > > reasons > > >>> to > > >>> > do > > >>> > > > > >>> otherwise" or simply saying - "I know it could be done > this > > >>> way > > >>> > > but I > > >>> > > > > >>> think otherwise" etc. etc. But sometimes I miss that > there > > is > > >>> a > > >>> > > > > >>> comment that I have not reacted to, I skipped it > > unconsciously > > >>> > etc. > > >>> > > > > >>> > > >>> > > > > >>> I think having "some" kind of reaction to comments and > > >>> deliberate > > >>> > > "I > > >>> > > > > >>> believe the conversation is resolved" is a very good > thing > > and > > >>> > > having > > >>> > > > > >>> the author making a deliberate effort to "mark the > > >>> conversation > > >>> > as > > >>> > > > > >>> resolved" is a sign it's been read, though about and > > >>> consciously > > >>> > > > > >>> reacted to. > > >>> > > > > >>> > > >>> > > > > >>> I've learned recently that you can add protection rule > that > > >>> will > > >>> > > > > >>> require all conversations on PR to be resolved before > > merging > > >>> > it, I > > >>> > > > > >>> even went to a great length to create (and get merged) a > > PR to > > >>> > ASF > > >>> > > > > >>> infra to enable it via .asf.yml feature > > >>> > > > > >>> (https://github.com/apache/infrastructure-p6/pull/1740) > - > > so > > >>> we > > >>> > > can > > >>> > > > > >>> enable it now by a simple PR to our .asf.yaml enabling > it. > > >>> > > > > >>> > > >>> > > > > >>> I'd love to try it - but of course it will have to > change > > a > > >>> bit > > >>> > > the > > >>> > > > > >>> workflow of everyone, where the author (or reviewer, or > > >>> > maintainer) > > >>> > > > > >>> will have to mark all conversations as resolved > > deliberately > > >>> > before > > >>> > > > > >>> merging PR. > > >>> > > > > >>> > > >>> > > > > >>> I'd love to enable it - at least to try and see how it > can > > >>> work - > > >>> > > but > > >>> > > > > >>> I understand it might add a bit of burden for everyone, > > >>> however, > > >>> > I > > >>> > > > > >>> think it might be worth it. > > >>> > > > > >>> > > >>> > > > > >>> WDYT? > > >>> > > > > >>> > > >>> > > > > >>> J. > > >>> > > > > >>> > > >>> > > > > >>> > > >>> > > > > --------------------------------------------------------------------- > > >>> > > > > >>> To unsubscribe, e-mail: > dev-unsubscr...@airflow.apache.org > > >>> > > > > >>> For additional commands, e-mail: > > dev-h...@airflow.apache.org > > >>> > > > > >>> > > >>> > > > > >>> > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > --------------------------------------------------------------------- > > >>> > > > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > >>> > > > > > For additional commands, e-mail: > dev-h...@airflow.apache.org > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > >>> --------------------------------------------------------------------- > > >>> > > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > >>> > > > > For additional commands, e-mail: dev-h...@airflow.apache.org > > >>> > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > > >>> --------------------------------------------------------------------- > > >>> > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > >>> > > > For additional commands, e-mail: dev-h...@airflow.apache.org > > >>> > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >> > > >
