reviewer attribution

[David Holmes]

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 :)

Cheers,
David

> -- 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
>>> work.
>>>
>>> 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 
>> suggests.
>>
>> Cheers,
>> David
>> -----
>>
>>> 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:
>>>
>>> https://github.com/openjdk/jdk/pull/1003
>>>
>>> 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".
>>>
>>> Roland.
>>>
>