Approving / Rejects PRs on GitHub when not a reviewer?
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.
+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 <webkit-dev@lists.webkit.org> 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
On Tue, Nov 28 2023 at 10:27:41 AM -0800, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> 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.
I have also seen cases of PRs rejected, asking the author to do more work, that I didn’t feel was necessary.
Hm, I wonder if these situations were exceptions rather than common situations? In general, I would expect that if a non-reviewer is requesting changes on a pull request, they are *probably* justified in doing so, so I'm much less concerned about non-reviewer use of "Request changes" than I am about non-reviewer use of "Approved." The submitter can always argue against the requests, after all. It's very common for a reviewer to request something and for the change author to push back. We usually agree in the end. :) I don't have a strong opinion on this. Michael
On Nov 28, 2023, at 11:51 AM, Michael Catanzaro <mcatanzaro@redhat.com> wrote:
On Tue, Nov 28 2023 at 10:27:41 AM -0800, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> 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.
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-... Thanks, Pascoe
On Nov 28, 2023, at 12:02 PM, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:
On Nov 28, 2023, at 11:51 AM, Michael Catanzaro <mcatanzaro@redhat.com> wrote:
On Tue, Nov 28 2023 at 10:27:41 AM -0800, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> 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
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
(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 <webkit-dev@lists.webkit.org> 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
participants (6)
-
Chris Dumez
-
Darin Adler
-
J Pascoe
-
Michael Catanzaro
-
Tim Horton
-
Tim Nguyen