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]
