Great questions. More context then, since you asked:
The main benefits I see:
1) the person merging the PR will not merge PRs when conversations are
still unresolved manually. Some of the PRS have long conversations and
I had many times - for example - cases where I missed that someone
made a comment or question which was important to raise but not
important enough to mark PR as "request changes".
2) the PR author will see clearly that the PR is not ready to merge if
there are unresolved conversations and will have to make an action to
make the PR mergeable - without anyone who comes and takes a look at
the PR and says ("Hey what do you think ? Is your problem solved?") It
will be clearly on the author to make it "ready" for the person who is
merging the PR.
3) it turns "starting conversation" into "soft request for changes".
There are many PRs where committers mark things as "request for
changes" and then they do not realise their comments have been
addressed. There are many, many cases where such PRs are "hanging"
without taking the "request for changes" off and it would be as easy
as "resolve comment".
Point 3) is actually the most important. It basically adds two level
of "request for changes" that the author of PR (or sometimes person
merging it, or sometimes the one who requested the changes) should
somewhat address:
* STRONG: Request for changes is "I strongly want this to be fixed
and please let me know when done that it was addressed so that I can
verify it"
* WEAK: I think you should make some changes but I TRUST you that you
will do them, let me just leave a comment so that you react and do
something about, but if you think you addressed my comment, you are
free to proceed.
I think the WEAK comment is super powerful in cases that are
"somewhat" important but not crucial. It gives freedom, responsibility
and more empowerment to the author of the PR, while keeping things in
"make sure you look at it camp". I would not use it for super
important things, and I would mostly use it for people who I know and
trust already. Trust and Empowerment is really the key word here. It
gives the one who click "Resolve" not only the power but also great
responsibility in assessing that indeed, the conversation is resolved.
And just to explain - it's not something that I came out of the blue.
This proposal and PR in INFRA is a direct result of listening to the
podcast on "Real Python":
https://realpython.com/podcasts/rpp/183/ "Exploring Code Reviews in
Python and Automating the Process" - by the authors of Sourcery. They
explain there in more detail why this works and how the motivations it
comes from. If you have not watched it, it gives much more background
and deep insight into how the code review process works. This is where
I've learned from that Github Allows you to use "require comments to
be resolved" branch protection.
J
On Tue, Dec 19, 2023 at 4:44 PM Vincent Beck <vincb...@apache.org> wrote:
>
> 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org
For additional commands, e-mail: dev-h...@airflow.apache.org
