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 <[email protected]> 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 > <[email protected]> wrote: > > > +1 > > > > On Tue, Dec 19, 2023 at 9:36 AM Pierre Jeambrun <[email protected]> > > 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 <[email protected]> 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 <[email protected]> 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 < > > > [email protected]> > > > > 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 <[email protected]> > > > 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: [email protected] > > > > > >>> For additional commands, e-mail: [email protected] > > > > > >>> > > > > > >>> > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > To unsubscribe, e-mail: [email protected] > > > > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > To unsubscribe, e-mail: [email protected] > > > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: [email protected] > > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > > >
