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
