reviewer attribution

[Kevin Rushforth]

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. 
It seems to me that the drawbacks outweigh any benefit.

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