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. 
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.
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.
@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.
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.
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. 
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
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.
As a non-core contributor who has been regularly submitting constructive reviews recently, I would have to disagree with this proposal. However, I do agree with the idea of rejecting non-core approvals without feedback, and doing as much as possible to communicate to contributors that their reviews are valuable primarily for constructive feedback, instead of the approval itself.
There have been a few times where I’ve been able to make suggestions to first-time contributors with regards to the CLA (in case they have to manually update the status, like I did) or patchcheck process. Having contributors assist with this rather than core devs helps to save valuable time.
With regards to the review process though, I think that it might be beneficial to have a intermediate position within the community similar the “triager” role that specializes in reviewing PRs from from non-core contributors. They could potentially help with assisting first-time contributors, adding labels (such as skip news and skip issue for minor ones), and attaching core devs as reviewers to unassigned PRs.
An issue with the existing system might be the automatic label changing from awaiting review to awaiting core review. At the moment, the label is changed whenever anyone adds a review to an issue, even if the person reviewing the changes chooses the request changes option. At the least, this should be fixed so that the label is only changed upon an approval. Another option might be to have that ability reserved to that specific intermediary role, but that’s a bit more long term since that role does not currently exist.
Edit: As for constructive reviews from non-core developers, I feel that this one might serve as a decent example: https://github.com/python/cpython/pull/14593.
Why “non-core”? I don’t see a justification for making a difference between core devs are non-core devs here. If anything, reviews from core devs should be held to a higher standard as they are more important and could serve as examples for others.
This proposal (which I have already retracted earlier) was about relatively unknown people just clicking the approval button without saying anything further.
It is different if you know someone’s review standards from 100 previous issues. This also applies to known contributors who are not core devs.
Specifically I just mean rejecting the non-core reviews who do not include any commentary in their feedback. Generally if a core developer approves without submitting direct feedback, they are saying “yes, this change is okay to merge” and providing the final stamp of approval. However, a new contributor doing so doesn’t accomplish too much. I’ve seen a few newer members of the community submitting the approve button without a word, and this doesn’t really make the process of making the final review any easier for the core devs (which seems to be the whole point of non-core reviews).
What Stefan said sums up the target audience (not every non-core dev review):
To make this less restrictive to contributors, my idea was to require some feedback (even it’s just a generic “Thanks for the contribution” message). If a non-core hit submit without any message, they would receive a message from the bot and the review wouldn’t go through.