In the same spirit as More granular commit access control via merge bot, I’d like to discuss a proposal to reduce PR lead time and improve the experience for authors and reviewers:
Require an approval from a reviewer who is not a committer for a PR to be merged.
Wait, what?!? How could adding more obstacles improve the PR review situation? Let me explain:
For a code contributor, getting a committer to look at your PR is everything. A non-committer can give helpful feedback, in theory - but usually, it’s the committer who either merges your changes or tells you the Nix incantation that will make your derivation worthy.
While non-committers are encouraged to review PRs, there remains little incentive to do so:
Since committer review is necessary always, non-committers can feel unwanted in a PR. Or, it feels like wasted effort.
For a contributor with PRs waiting for review, becoming a committer is not a solution. Code contributors want other people to become committers. (Plus, when it’s an expectation for a committer to have 50+ merged PRs and you’re struggling to get anything reviewed, it’s a discouraging situation.)
Non-committer reviews are valuable, but the system we have now fails to make non-committers feel valued.
The idea of this proposal is to increase reviewer participation by making non-committer review a crucial part of the process.
- Authors will seek out reviews specifically from non-committers.
- Non-committer approval meaningfully moves a PR forward.
- Authors are more incentivized to address non-committer comments.
- Non-committers get satisfaction from all of the above.
- Non-committers get more practice reviewing PRs.
Reviews are valued → Reviewers have fun → More reviews → Shorter backlog.
A few details:
- Exceptions may be necessary for security fixes, urgent reverts, and other emergencies.
- It should be easy to tell at a glance whether a PR is eligible to merge.
- It should be easy to see a list of PRs requiring first-pass reviews (or PRs already approved).
Concretely, I propose adding a GitHub status check that checks whether any approver is not a member of
nixpkgs-committers. This check should be configured as not required in GitHub (at least initially). Committers should (in the RFC2119 sense) refrain from merging PRs where this status check has not passed (but committers should always feel free to leave a review and approval.)
I don’t know much about automation on GitHub, but adding labels based on approval status seems eminently doable.
Thanks for reading my proposal!