I like the idea, at least double checking what the comments were is a good cause. +1 to incorporating this.
On 2023/12/19 13:12:05 Jarek Potiuk 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
