Disable approving reviews of external contributors

I wonder if it is possible do disable the “approve” button for external GitHub reviews. Many people click it without articulating themselves.

It is one thing for a core dev (who you know) to do that, but coming from a third party it does not help.

GitHub of course encourages this because a review shows up as an “activity”.

I understand exactly what you mean because I see it too. However, I do think most people are genuinely trying to help out and aren’t just trying to increase their GitHub score. They may not know how to do a code review and they may be coming from another project where changes are merged quickly and without as much consideration as in CPython, so they may think they are helping.

In the current workflow, the PR is initially opened as ‘Awaiting Review’, which is ideally a review from someone who understands the module being reviewed. We would hope that it would be a helpful review, with constructive comments or an explanation of why they approve it. Perhaps one solution here is to describe that better in the devguide (current section that addresses it) and if someone sees a ‘serial approver’, they can ask them to read the expectations there. But, who would that ‘someone’ be? If we developed a well-written template, then anyone could send it, but, as always, the human side is the hard part. :slight_smile:

The difficulty with removing that button right now is that triagers (people with more trust on the bpo) don’t have any special authority on GitHub. So, useful reviews and approvals from people like Paul Ganssle and Karthikeyan Singaravelan and many others would be lost if the ability to approve was taken away. Until the triagers get permissions on GitHub (or get the commit bit), I think it’s easiest to ignore the mass approvals or send them a note pointing them to the devguide.

3 Likes

I have moved this post to core-workflow.

They still can set the ‘stage’ field to ‘commit review’ (including a comment explaining why they think the patch is ready) on bugs.p.o. The purpose of the ‘commit review’ stage was to let core devs know that the patch is good enough to be reviewed/committed by a core developer.

I don’t think most of the core devs are subscribed to python/cpython notifications, so unless someone has specifically pinged them on the PR or set the ‘staging’ field to ‘commit review’ on bugs.p.o., “approvals” of contributors on GitHub won’t be noticed.

I’m not sure that disabling the “approve” button would be a viable solution. Something like that would pretty much exclude a whole group of non core-devs from contributing to CPython.

I also think that overgeneralising and excluding “good” contributors just out of fear of “bad” ones, who only do things to score GitHub points, is not something we want as a community, which tries to be as including as possible. There may be other ways to prevent such situations, such as stricter rules and bans on people, that continuously review things just to get a nicer GitHub homepage.

Something like that could also prove harmful for the project itself in the long run. If core-devs are the only ones, that can review PRs, the project might go into a vicious circle, where no new people can be promoted to core-devs, just because they don’t have the ability to prove themselves as reviewers.

4 Likes

@csabella I’m sure people are trying to help, the GitHub comment is more general.

It isn’t a very big deal, I’m just getting about two PRs that are autoclosed (wrong branch, too many files) per week, and some of these reviews without any comment.

I should just ignore it, thanks for the perspective.

1 Like

Reviews from non core developers are valuable, I don’t think we should discourage it.

What we can do is provide guidance on how to properly review (not just simply click approve).

As core devs it is still in our own power/responsibility to do another pass of review, make our own assessment and then merge the PR.

1 Like

Yes, it’s true, I guess it comes down to communication, in some cases I’m just bewildered why anyone would “approve” without any rationale.

So let’s not change anything. :slight_smile:

We could have Bedevere or some bot close reviews with no comment and then message the person who left the review to please review again but leave some feedback (even if it is just words of encouragement).

That would be a possibility, on the other hand it’s one additional mail, like with the auto-closes. Is bevedere for anything important or only for auto-responding? Perhaps I should just filter all mails from the bot.

Bedevere sets various labels, asks the user to address requested changes, leaves messages on how to notify the bot that the PR author feels they are ready for another review, and then leaves an @ mention w/ label change for the reviewers to let them know that the PR is ready to review again.

So if you have a better mechanism to know when a PR is ready to review then you could get away with ignoring it. There is an feature request on Bedevere open to automatically re-request a review using GitHub’s mechanism for such things instead of having the bot do it. We could also discuss dropping this aspect of Bedevere all together if we think users are knowledgeable enough to re-request reviews now that GitHub provides such a mechanism (but that should be a separate topic of discussion if we want to talk about tweaking that part of the workflow).

Indeed. Positive feedback encouraging people to do the right thing is always preferable to censure.

Thanks. It’s exactly what I mean. We must make core devs less special, not more special.

Recently, I got more and more reviews from non core devs, and honestly it helps me to be more confident to merge my own changes. I expect that when someone approves a change, they read it and didn’t notice anything obvious. Sometimes, they spot a typo or ask an accurate question.

But core devs are special. Maybe we should better highlight that only core devs are allowed to merge a change, and that approvals from core devs are more important since we expect core devs to pay attention at more things: ref leaks, backward compatibility and many other small things.

My call here would be to nothing :slight_smile:

I’m using GitHub nicknames to check if a reviewer is a core dev or not. Usually, when I don’t know a reviewer, they are not a core dev. And that’s fine.