review PR merge policy

[belm0]

From contributing docs:

We do have one rule, which is the same one most F/OSS projects use: don't merge your own PRs. We find that having another person look at each PR leads to better quality.

"Having another person look at each PR" is not controversial. However it's not obvious why this necessitates a "don't merge your own PR" rule. Requiring approval from a Trio organization member may be sufficient for our goal, and GitHub allows such policy to be enforced on a project.

While there is also the external / first time PR contributor case, this rule is only relevant to PR's authored by existing organization members (which by Trio's contribution policy is anyone with a PR merged in the past).

recent/consistent contributors for input: @njsmith @pquentin @smurfix @oremanj

[belm0]

My take: a policy requiring approval from a Trio organization member is easy to understand, sufficient for the goal of having all repository changes reviewed, and can be enforced by our VCS. Let's give the participants of each PR the freedom to decide who pushes the merge button.

I prefer the default expectation that PR author merges. It avoids the problem of a PR being merged before it's ready (e.g. author forgot to mention a TODO she wants resolved before merging, or after sleeping on it realized a problem with the PR, etc.).

[pquentin]

The relevant Gitter dicussion: https://gitter.im/python-trio/general?at=5d004bdca30be21ff9f3b0a3

I don't personally care too much about this, but I'd like to avoid PRs that stay open just because they were "only" approved.

Maybe it's more important to clarify what the bar is for approving/merging. Should we try to test the code, or is it OK to just look at it?

Would it be useful to configure GitHub to require an approval before merging?

[ziirish]

I'm personally used to minimal approval requirements before being able to merge a PR.

Also I find @belm0 point about who knows best when a PR is ready quite valid though that would maybe require GitHub to disapprove any PR upon change (suppose a PR was approved before it was actually ready, then we would need it to be re-reviewed if it's updated).

Don't know if any of this is doable with GitHub though.

[njsmith]

@belm0, thanks for raising this. Like all process things, we can do whatever works for us, but as the project grows it's important that we revisit things and make sure everyone's on the same page!

I think there are two historical approaches that are common. We don't have to do either, but I think it's useful to think about them.

For corporate projects, there's generally a small group of folks working on the project, who are expected to have significant expertise and training, and are all trusted. Everyone who submits PRs also has merge rights, and everyone knows the rules. Because people are working on this full time and working to external schedules, situations arise where you can and should do complex changes with careful coordination across multiple related PRs.

For your average open-source project, there's generally a large number of contributors, and a small number of trusted maintainers/reviewers. Most of the time, people who submit PRs don't have the right to merge their PR, and there's a reasonable chance that by the time the PR gets reviewed, the original submitter isn't even around anymore. Contributors have a wide variety of backgrounds and skill levels. Complex coordinated changes are more-or-less impossible, so they just don't happen.

So, it makes sense that corporate projects would tend towards the model where reviewers approve → original submitter hits merge: it doesn't really have any downsides in that context. But for your average open-source project, it doesn't work at all, because the original contributor likely can't merge. And even if that weren't an issue, that model requires that everyone have a clear understanding of what "approve" means, and that's not necessarily the case for open-source projects: when I see "so-and-so has approved this change", I don't know what so-and-so was thinking when they hit "approve". Some projects use it to mean "I looked and didn't see any problems, but I'm not confident enough to say it's ready to merge". Heck, random passerby on github will sometimes mark PRs approved even though they have no connection to the project at all.

Now Trio isn't quite like either of these stereotypical projects! Obviously we're an open-source volunteer project, but instead of having a tiny core group who does all the reviewing, we have our wide-open membership policy, which changes things some.

Some advantages of approve-by-merging:

• Works in all cases: a fair number of our PR are still from first-time contributors who can't merge (and wouldn't know what an "approve" means anyway). We could use approve-by-merging for first-time authors and approve-by-approving for non-first-time authors, but that makes our review process more complicated (reviewers have to check which case they're in every time).
• More streamlined: it's fewer steps, so less process overhead. (This is also why Trio doesn't have github set to mandate approvals before merging – if I know it's good to merge, I want to do that in one click; if I have to add an approval first it's like an extra 4 clicks or something.)
• Less chance for miscommunication: everyone knows what happens if you click merge :-). But as we've seen here, not everyone uses "approve" the same way. Of course, the solution is just to communicate and get everyone on the same page – but since we have new contributors coming in all the time, we'll have to keep teaching everyone whatever we decide, and simpler processes are easier to teach.

Some advantages of approve-by-approving (mostly drawn from this thread):

• It gives the original author a chance to triple-check things before merging, and make last minute corrections.
• It reduces the pressure on the reviewer ("ultimately person hitting merge button is responsible for build or runtime breakage. It seems like too much responsibility for a reviewer.")
• It gives the original author a chance to decide which kind of merge to use (e.g. regular merge vs squash merge)
• It gives the original author control over the detailed timing, e.g. in case there are several PRs that have to be merged in a specific order

I might be biased because of my experience being more in the open-source world, but tbh I don't find these super compelling?

• If the original author said it was ready for review, and then the reviewer thought it looked good too, then we can be reasonably confident it's ready. Never 100%, of course, but adding a third-layer of "last minute checking" seems like it has diminishing returns.
• If things break then of course they need to be fixed. I would hope that the reviewer and the original author would both be likely to help with this in any case, since they're the folks who know the most about what was merged. But if they're not around someone else will do it. And if we require both the original author and the reviewer to sign off, then I don't see how it makes much difference which of them actually hits the button?
• I mostly don't care about squash vs regular merges, it's just not that important compared to other workflow issues
• We generally don't do complex coordinated changes, or if we do then the author can always explain the issue in the PR and work out a plan

So personally the approve-by-merging approach still seems more attractive to me as a default, and we let people negotiate other approaches on an ad hoc basis. But I'd like to hear what other people think.

[One thought that did occur to me during this: for authors who are relatively new contributors, but on their 2nd or 3rd PR, so they're a member, I may start saying 'this looks good to me, do you want to hit the green merge button?', as a way to encourage people to get comfortable hitting that button in a controlled environment.]

[belm0]

Thank you for considering.

Even in a corporate setting (and especially in a large ones), repo's may have relatively few owners vs. contributors (other teams trying to submit fixes and enhancements).

Granted that the meaning of "approve" is defined (or not) by a project's policy, but requiring an approval at least ensures a 2nd person is involved and limits the amount of damage one person can do (typically due to a lack of understanding rather than malicious intent). Especially given Trio's inclusive policy of giving merge rights to all contributors, requiring a 2nd person seems worth considering.

Requiring an approver doesn't preclude a Trio member from merging a random contributor's PR (it needs more clicks as Nathaniel points out).

I noticed today that python-hyper (at least wsproto) requires a PR approval-- it's not odd, and in fact seems a lot more common than "you can't merge your own PR".