+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 > > > > >
