Thanks for the clarification Jarek, I see the value in it. It's a +1 from me to try it out.
-- Regards, Aritra Basu On Wed, Dec 20, 2023, 7:04 PM Jarek Potiuk <ja...@potiuk.com> wrote: > Aritra: > > > I think it's a fine idea to experient with to see if the advantages > outweigh the extra friction. I do have a question and forgive my ignorance, > but this I assume isn't restricting a commit author from resolving the > comments in cases where they either think it doesn't need fixing or if it's > perhaps something for a follow-up pr? What I mean is it isn't gonna enforce > who can resolve a comment thread? > > Not at all. Just to repeat from the earlier answer: > > 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. > > I do not think we need (and that was not a suggestion) to add > more bureaucracy or process around it. It's purely a gatekeeper to make > sure that all the comments are "marked as resolved" before merging the PR. > > Maybe that's not clear - but in my proposal, I do not want to decide, nor > mandate whether it's the author, reviewer or even another maintainer who > decides to merge the PR who closes the conversation. This is really the > part of "adult trust" that I was talking about. > > As I see it, we are really putting the trust in the hands of those who mark > the comment as resolved (whether it's author, reviewer, or person who > merges it). And we trust them that their intention is "yes, I believe that > conversation is resolved". > We still have other ways when we really want to block things before we > re-verify them as maintainers (i.e. "request changes") and this should be > in our toolbox as well. And we should use it when needed of course. > > But what I really like in this case is that: > > a) it allows bystanders, non-committers to have an opinion there which is > non-blocking but eventually requires some reaction from someone - author or > maintainer (even if it is - "nope look up, it's explained above" or even > just "resolving" without comment in some obvious cases. Yes, there is a > small risk of slowing things down/spamming but we can always contain that > when it happens. We have tools for that. > > b) I think it will result (and this is also what the experience of Sourcery > creators in the podcast I mentioned above is) results with less number of > stale "request changes" and people feeling more empowered and trusted to do > the right things. It's also more difficult for people - because they have > to do something with the trust that is given to them. But I think it's the > right thing to do and eventually might result in faster turnaround on PRs > (or at least that's one the mechanisms described in the Podcast as "working > this way" eventually. > > I really recommend listening to the podcast - > https://realpython.com/podcasts/rpp/183/ - while I do not necessarily > agree with everything there, this one got stuck in my head when I thought > that it might, indeed, work as described. > > J. > > > > On Wed, Dec 20, 2023 at 2:05 PM Aritra Basu <aritrabasu1...@gmail.com> > wrote: > > > I think it's a fine idea to experient with to see if the advantages > > outweigh the extra friction. I do have a question and forgive my > ignorance, > > but this I assume isn't restricting a commit author from resolving the > > comments in cases where they either think it doesn't need fixing or if > it's > > perhaps something for a follow-up pr? What I mean is it isn't gonna > enforce > > who can resolve a comment thread? > > > > -- > > Regards, > > Aritra Basu > > > > On Wed, Dec 20, 2023, 4:09 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > If there are no more big objections, I will enable it after the 26th of > > > December to see how it can work. > > > > > > This will also be a naturally slower period due to Holidays and New > > Year's > > > eve, so even if it disrupts someone initially it will have a smaller > > radius > > > blast potentially. I will also make sure to communicate it in a few of > > our > > > channels that we are experimenting with and be on the lookout to help > > > people if they will have problems with it. > > > > > > J. > > > > > > > > > On Wed, Dec 20, 2023 at 8:31 AM Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > > > Amogh: > > > > > > > > > Should we enforce creating a follow up Github issue in our case for > > the > > > > same? This would solve the issue of unfinished PRs > > > > while also giving confidence that the comments won't be ignored. > > > > > > > > I think we already do that in a number of cases. And I would not > > enforce > > > > it, especially that (unlike "do not merge until resolved") it's not > > > > automatable. Here I would actually say what Ash said about being > > "adult". > > > > The whole idea is not because we do not trust people to do the right > > > thing > > > > - it's there to make sure that we do not forget about some open > > > > conversation (and that goes to both - author and reviewer). > > > > > > > > For the author, this will be a sign to see that there is "something" > to > > > do > > > > when there are conversations that are still open. For the reviewer, > it > > > > should also be a sign "I can merge it now, really" or "let's see if > > there > > > > is anything left to address here, I have not realised there is this > one > > > > more comment not addressed". > > > > > > > > On Wed, Dec 20, 2023 at 5:16 AM Amogh Desai < > amoghdesai....@gmail.com> > > > > wrote: > > > > > > > >> I generally like the idea of adding this rule but I have a few > > concerns > > > >> around the amount of unfinished/incomplete PRs this could lead to. > > > >> > > > >> We generally follow such things in my org where we try to resolve > all > > > the > > > >> comments from the maintainers > > > >> before performing a merge. But what we also do is that if a > > comment/ask > > > >> from maintainer is outside bounds for that > > > >> a particular PR, we generally create follow up JIRAs and then follow > > up > > > on > > > >> it later. > > > >> > > > >> Should we enforce creating a follow up Github issue in our case for > > the > > > >> same? This would solve the issue of unfinished PRs > > > >> while also giving confidence that the comments won't be ignored. > > > >> > > > >> > > > >> Thanks & Regards, > > > >> Amogh Desai > > > >> > > > >> On Wed, Dec 20, 2023 at 5:11 AM Jarek Potiuk <ja...@potiuk.com> > > wrote: > > > >> > > > >> > 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 > > > >> > > > >>> > > > > > > >> > > > >>> > > > > > > >> > > > >>> > > > > > >> > > > >>> > > > > >> > > > >>> > > > >> > > > >> > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > >
