david.holmes at oracle.com
Thu Nov 19 00:38:29 UTC 2020
On 18/11/2020 11:27 pm, Kevin Rushforth wrote:
> Manually adding a reviewer who has made comments, but not formally
> approved via the UI, seems a useful feature. I don't think tht manually
> adding a reviewer who has done a review requesting changes, which is the
> way you indicate that your comments are blocking, is such a good idea.
That depends on the nature of the comments/changes.
> It seems to me that the drawbacks outweigh any benefit.
The only drawbacks would be if someone misused the capability - and that
is a people problem that can be addressed through education.
The main issue here is not giving credit to reviewers when it should be
given**, and not making the process too onerous for those reviewers by
requiring that they always click "approve" after the final commit.
** not that that is at all relevant in my case :)
> -- Kevin
> On 11/17/2020 6:47 PM, David Holmes wrote:
>> On 17/11/2020 6:58 pm, Roland Westrelin wrote:
>>> Hi Skara Developers,
>>> My experience with Skara has so far been painless. Thanks for the great
>>> I noticed one of my PRs has 3 reviewers listed on the right hand size
>>> under "reviewers" but a single one listed in the comments. Only that one
>>> reviewer has approved the change with the github web interface. Change
>>> is cleared for integration so I could push it without proper reviewer
>>> attribution. I tried the /reviewer command to force the system to record
>>> the 2 other reviewers but that doesn't help.
>> I agree that this is a problem. There is a case to be made that
>> reviewers just have to get used to going back to the PR and clicking
>> "Approve", and that developers have to wait until the reviewers have
>> done so. But that is contrary to our position that we don't block
>> integrations until the approvals are in place, to allow for trivial
>> final updates.
>> It is common (for me at least) to do a review and select "comment" or
>> "changes requested" for minor issues. They then get fixed, but I don't
>> get listed as a Reviewer unless I go back and rubber stamp them. I've
>> started doing that but it is a bit of a pain.
>> On the flip side someone who just makes a passing comment shouldn't
>> automatically get the credit, or responsibility, of being a reviewer.
>> I don't see a solution other than allowing a manual override as Roland
>>> To me, this feels like a step backward from what we had with mercurial
>>> where we were in full control of the reviewer list. There's a risk that
>>> even careful developers could push a change without proper reviewer
>>> attribution. Also, in the openjdk project, we largely trust developers
>>> for doing the right thing. In that spirit, allowing the /reviewer
>>> command full power on the reviewer list would make sense for the rare
>>> case where it's obvious the change is good to go but someone hasn't
>>> explicitly approved it. I feel it's better than having to chase all
>>> reviewers to have them click a button or integrating a change without
>>> all involved reviewers credited.
>>> The PR is:
>>> but I emailed reviewers that were not credited to ask them to approve
>>> the change so the reviewer list is in the process of being "fixed".
More information about the skara-dev