Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Ryosuke Niwa via webkit-dev]

I don't think non-reviewers should be able to reject/block PRs as well.- R. NiwaOn Nov 29, 2023, at 07:45, Chris Dumez via webkit-dev  wrote:FYI, our official documentation on WebKit.org says:```Making unofficial reviews before you become a reviewer is encouraged. This is an excellent way to show your skills. Note that you should not put r+ nor r- on patches in such unofficial reviews.```I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d start enforcing this policy again.Having the tools help us would be great but I don’t think it stops us from enforcing our own policies like we used to.On Nov 28, 2023, at 1:58 PM, Michael Catanzaro  wrote:On Tue, Nov 28 2023 at 01:23:12 PM -0800, Chris Dumez via webkit-dev  wrote:I´m on board if it also cancels PR rejections from non-reviewers, not just approvals. I don´t see how approvals differ from rejections.Sure. It doesn't really matter whether rejections are canceled or not, because the important part of the rejection is the comments that were added, not the rejection status itself. A rejection from a non-reviewer is not effective anyway, so it's fine to have a bot clarify that.Michael___webkit-dev mailing listwebkit-dev@lists.webkit.orghttps://lists.webkit.org/mailman/listinfo/webkit-dev___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

> On Nov 28, 2023, at 4:06 PM, Jean-Yves Avenard  
> wrote:
> 
> Hi
> 
>> On 29 Nov 2023, at 9:44 am, Chris Dumez via webkit-dev 
>>  wrote:
>> 
>> FYI, our official documentation on WebKit.org  says:
>> ```
>> Making unofficial reviews before you become a reviewer is encouraged. This 
>> is an excellent way to show your skills. Note that you should not put r+ nor 
>> r- on patches in such unofficial reviews.
>> ```
>> I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on 
>> bugzilla translates to no approve / deny PRs on GitHub. So I simply wish 
>> we’d start enforcing this policy again.
>> 
>> Having the tools help us would be great but I don’t think it stops us from 
>> enforcing our own policies like we used to.
> 
> Personally, I’ve been requesting non-official reviewers to review my patches 
> because I know that their skill set is perfectly matched (and it will help 
> make them official reviewer)
> 
> Having them giving r+ explicitly is, I find, easier to spot than looking 
> through the often busy GitHub page to find the comments.

Even if someone approves, you’d still need to find the comments. r+ with 
comments is super common.

> 
> Could we relax the ability to give informal r+ review to people with commit 
> rights? 

They can just leave a comment saying the patch looks good. They don’t have to 
approve the PR. Approving the PR means nothing since it won’t let you merge 
your PR.
People are expected to wait for an official review to trigger the merge queue, 
we’re currently making it harder than it needs to be if we have an official 
review or not.

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Jean-Yves Avenard via webkit-dev]

Hi

> On 29 Nov 2023, at 9:44 am, Chris Dumez via webkit-dev 
>  wrote:
> 
> FYI, our official documentation on WebKit.org  says:
> ```
> Making unofficial reviews before you become a reviewer is encouraged. This is 
> an excellent way to show your skills. Note that you should not put r+ nor r- 
> on patches in such unofficial reviews.
> ```
> I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on 
> bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d 
> start enforcing this policy again.
> 
> Having the tools help us would be great but I don’t think it stops us from 
> enforcing our own policies like we used to.

Personally, I’ve been requesting non-official reviewers to review my patches 
because I know that their skill set is perfectly matched (and it will help make 
them official reviewer)

Having them giving r+ explicitly is, I find, easier to spot than looking 
through the often busy GitHub page to find the comments.

Could we relax the ability to give informal r+ review to people with commit 
rights? 

(And it’s also great to be able to provide stats later to say see, that person 
did XX informal reviews :) )

Jean-Yves___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

FYI, our official documentation on WebKit.org  says:
```
Making unofficial reviews before you become a reviewer is encouraged. This is 
an excellent way to show your skills. Note that you should not put r+ nor r- on 
patches in such unofficial reviews.
```
I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on 
bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d 
start enforcing this policy again.

Having the tools help us would be great but I don’t think it stops us from 
enforcing our own policies like we used to.

> On Nov 28, 2023, at 1:58 PM, Michael Catanzaro  wrote:
> 
> On Tue, Nov 28 2023 at 01:23:12 PM -0800, Chris Dumez via webkit-dev 
>  wrote:
>> I´m on board if it also cancels PR rejections from non-reviewers, not just 
>> approvals. I don´t see how approvals differ from rejections.
> 
> Sure. It doesn't really matter whether rejections are canceled or not, 
> because the important part of the rejection is the comments that were added, 
> not the rejection status itself. A rejection from a non-reviewer is not 
> effective anyway, so it's fine to have a bot clarify that.
> 
> Michael
> 
> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Michael Catanzaro via webkit-dev]

On Tue, Nov 28 2023 at 01:23:12 PM -0800, Chris Dumez via webkit-dev 
 wrote:
I’m on board if it also cancels PR rejections from non-reviewers, 
not just approvals. I don’t see how approvals differ from 
rejections.


Sure. It doesn't really matter whether rejections are canceled or not, 
because the important part of the rejection is the comments that were 
added, not the rejection status itself. A rejection from a non-reviewer 
is not effective anyway, so it's fine to have a bot clarify that.


Michael


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Jonathan Bedard via webkit-dev]

> Hi,
> 
> Back in the Bugzilla days, only reviewers were allowed to r+ or r- patches. 
> Non-reviewers were - of course - encouraged to do informal reviews but they 
> would do so by leaving comments. They would never r+ / r-.
> 
> Since we?ve moved to Github, it seems we have become a lot more lax about 
> this and I have seen non-reviewers approve and reject PRs, not just leaving 
> comments.
> My understanding is that there is no way to prevent this with Github but 
> could we at least make it a policy that non-reviewers should not approve / 
> reject PRs and only leave comments instead?

To provide some context, one of the reasons I didn’t attempt to exactly 
replicate Bugzilla’s behavior when porting our review mechanisms to GitHub was 
that Bugzilla only allows a single reviewer, which meant that an r+ would 
naturally clobber an r-, regardless of who gave the r- initially. Even in 
Bugzilla, non-reviewers could give r+es or r-es, it’s just that Commit-Queue 
wouldn’t respect them.

> The reason I would like us to make enforce this rule is that I find it 
> confusing. We have a lot of new comers in the project and I do not always 
> know if a person is a reviewer or not yet. I imagine it may be even more 
> confusing for non-Apple people.

I agree with this, especially since folks GitHub usernames are not always 
obvious.

> I have in some cases not reviewed patches because I had seen the "green 
> check? and thought the PR already had been approved.
> I have also seen cases of PRs rejected, asking the author to do more work, 
> that I didn?t feel was necessary.
> There is no easy way from the GitHub UI to tell if the person who 
> approved/rejected your PR is actually a reviewer, as far as I know.

I think it’s fair for a non-reviewer to “Request Changes” on a PR, it’s often 
the case that a non-reviewer has comments that should block a change from 
landing. If a reviewer (or even the PR author) thinks that the blocking comment 
has been addressed, they can “re-request review” which will get-rid of symbol 
until the account whose review is requested re-reviews the PR.

I think this also points to the right way to address the confusion for r+es 
(which I think are the bigger issue): if we formally make our policy that 
non-reviewers should not leave a persistent “Approved” checkmark on a PR, we 
could have EWS listen for PR approvals and simply request a re-review any time 
a non-reviewer approves a PR. In practice, this will mean that a non-reviewer 
which approves a PR will have their approval nearly instantly removed by a bot. 
We could even have the bot comment something like “Unofficial r+ from 
non-reviewer ” to make exactly what’s going on clear.

The issue with a policy change like this one is that it’s WebKit reviewers who 
deeply understand the policies of the WebKit project, but it’s non-reviewers 
who need to change their behavior. If we don’t back this policy clarification 
with a tools change, I think non-reviewers are unlikely to be aware project 
policy has changed.

> 
> What do you think?
> 
> Thanks,
> Chris Dumez.

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Tim Nguyen via webkit-dev]

(I already answered this, but that message failed to reach the mailing list, 
sorry to the folks receiving my reply twice)

Everyone in the WebKit Github organization gets a green checkmark afaik (that’s 
just how the Github UI works), so as of right now that means everyone in 
contributors.json with a Github username.

> On Nov 28, 2023, at 12:34 PM, Darin Adler via webkit-dev 
>  wrote:
> 
>> On Nov 28, 2023, at 3:02 PM, Chris Dumez wrote:
>> 
>> FYI, my understanding is that the person gets a *green* checkmark when the 
>> person is present in contributors.json (common case), even if not marked as 
>> a reviewer in that file.
> 
> Does anyone know why we chose green for all contributors rather than green 
> for reviewers?
> 
> — Darin
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Darin Adler via webkit-dev]

> On Nov 28, 2023, at 3:02 PM, Chris Dumez wrote:
> 
> FYI, my understanding is that the person gets a *green* checkmark when the 
> person is present in contributors.json (common case), even if not marked as a 
> reviewer in that file.

Does anyone know why we chose green for all contributors rather than green for 
reviewers?

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[J Pascoe via webkit-dev]

I think we could fix this by making EWS smarter. 

There is GitHub API to list and “dismiss” reviews on a PR that repository 
admins have permissions to use. 

https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#dismiss-a-review-for-a-pull-request

Thanks,
Pascoe

> On Nov 28, 2023, at 12:02 PM, Chris Dumez via webkit-dev 
>  wrote:
> 
> 
>> On Nov 28, 2023, at 11:51 AM, Michael Catanzaro  
>> wrote:
>> 
>> On Tue, Nov 28 2023 at 10:27:41 AM -0800, Chris Dumez via webkit-dev 
>>  wrote:
>>> The reason I would like us to make enforce this rule is that I find it 
>>> confusing. We have a lot of new comers in the project and I do not always 
>>> know if a person is a reviewer or not yet. I imagine it may be even more 
>>> confusing for non-Apple people.
>>> I have in some cases not reviewed patches because I had seen the "green 
>>> check” and thought the PR already had been approved.
>> 
>> +1, if there is a green checkmark, then I assume that person is a reviewer. 
>> It's really confusing when non-reviewers add the green checkmark.
> 
> FYI, my understanding is that the person gets a *green* checkmark when the 
> person is present in contributors.json (common case), even if not marked as a 
> reviewer in that file.
> They get a *grey* checkmark when they’re not present in contributors.json 
> (not common). Also, the difference between green and grey for this tiny 
> checkmark is super subtle.
> 
> That said, most of the instances where I saw it happened was with a green 
> checkmark.
> 
> Either way, I think non-reviewers should not approve/reject PR, just add 
> comments to avoid confusion.
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

> On Nov 28, 2023, at 11:51 AM, Michael Catanzaro  wrote:
> 
> On Tue, Nov 28 2023 at 10:27:41 AM -0800, Chris Dumez via webkit-dev 
>  wrote:
>> The reason I would like us to make enforce this rule is that I find it 
>> confusing. We have a lot of new comers in the project and I do not always 
>> know if a person is a reviewer or not yet. I imagine it may be even more 
>> confusing for non-Apple people.
>> I have in some cases not reviewed patches because I had seen the "green 
>> check” and thought the PR already had been approved.
> 
> +1, if there is a green checkmark, then I assume that person is a reviewer. 
> It's really confusing when non-reviewers add the green checkmark.

FYI, my understanding is that the person gets a *green* checkmark when the 
person is present in contributors.json (common case), even if not marked as a 
reviewer in that file.
They get a *grey* checkmark when they’re not present in contributors.json (not 
common). Also, the difference between green and grey for this tiny checkmark is 
super subtle.

That said, most of the instances where I saw it happened was with a green 
checkmark.

Either way, I think non-reviewers should not approve/reject PR, just add 
comments to avoid confusion.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Tim Horton via webkit-dev]

+1! The bugzilla-style “unofficial r=me” comment was much clearer for exactly 
these reasons.

> On Nov 28, 2023, at 10:27 AM, Chris Dumez via webkit-dev 
>  wrote:
> 
> Hi,
> 
> Back in the Bugzilla days, only reviewers were allowed to r+ or r- patches. 
> Non-reviewers were - of course - encouraged to do informal reviews but they 
> would do so by leaving comments. They would never r+ / r-.
> 
> Since we’ve moved to Github, it seems we have become a lot more lax about 
> this and I have seen non-reviewers approve and reject PRs, not just leaving 
> comments.
> My understanding is that there is no way to prevent this with Github but 
> could we at least make it a policy that non-reviewers should not approve / 
> reject PRs and only leave comments instead?
> 
> The reason I would like us to make enforce this rule is that I find it 
> confusing. We have a lot of new comers in the project and I do not always 
> know if a person is a reviewer or not yet. I imagine it may be even more 
> confusing for non-Apple people.
> 
> I have in some cases not reviewed patches because I had seen the "green 
> check” and thought the PR already had been approved.
> I have also seen cases of PRs rejected, asking the author to do more work, 
> that I didn’t feel was necessary.
> There is no easy way from the GitHub UI to tell if the person who 
> approved/rejected your PR is actually a reviewer, as far as I know.
> 
> What do you think?
> 
> Thanks,
> Chris Dumez.
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

Hi,

Back in the Bugzilla days, only reviewers were allowed to r+ or r- patches. 
Non-reviewers were - of course - encouraged to do informal reviews but they 
would do so by leaving comments. They would never r+ / r-.

Since we’ve moved to Github, it seems we have become a lot more lax about this 
and I have seen non-reviewers approve and reject PRs, not just leaving comments.
My understanding is that there is no way to prevent this with Github but could 
we at least make it a policy that non-reviewers should not approve / reject PRs 
and only leave comments instead?

The reason I would like us to make enforce this rule is that I find it 
confusing. We have a lot of new comers in the project and I do not always know 
if a person is a reviewer or not yet. I imagine it may be even more confusing 
for non-Apple people.

I have in some cases not reviewed patches because I had seen the "green check” 
and thought the PR already had been approved.
I have also seen cases of PRs rejected, asking the author to do more work, that 
I didn’t feel was necessary.
There is no easy way from the GitHub UI to tell if the person who 
approved/rejected your PR is actually a reviewer, as far as I know.

What do you think?

Thanks,
Chris Dumez.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev