"PRs must not be merged with pending reviews" section of our contribution policy

[Manishearth]

trafficstars

CONTRIBUTING.md states:

PRs must not be merged with pending reviews. If the PR author decides to make any substantial changes that go beyond of what the reviewers already approved, they can re-request an already accepted review after updating the PR.

In practice, this is not followed often. Personally, I treat the reviewers list as an ANY(codeowner) list, and if I actually care about getting specific approvals from specific people, I try to clearly specify it in the PR body (e.g. "This cannot land unless @sffc has looked at the whatever-whatever code"). Other team members seem to do the same.

We should discuss this policy, and change it if it is not serving us.

[Manishearth]

A couple beliefs/axioms I hold here:

• It is uncommon to need multiple reviews for a PR
  • Exceptions: changes that touch multiple components in a nontrivial way, changes that are particularly tricky, changes that are major
• I think it should be uncommon to need multiple reviews for a PR. We are a small team spread across timezones, and also:
• I do not consider it a huge risk for PRs to be landed when there may potentially be issues found by someone else. These can be fixed in a followup, and PRs can be reverted if they're entirely the wrong approach.
  • Reviewers should take care to say things like "This looks good to me, but I want @robertbastian to look at it" when they are unconfident in their reviews.
• I find little value in seeing what is on the reviewer list for a PR. Sometimes I notice myself on a list and remove myself in favor of a better reviewer, but aside from that the reviewer list does not signal much to me.
• I do find value in being able to specify a list of reviewers out of whom I wish to have a single approval.
• It is important for code to get approved by a de-facto codeowner. We don't do a great job of keeping CODEOWNERS up to date, and we have a couple people who are effectively whole-codebase codeowners, but regardless of what the file says, for a given PR it tends to be reasonably clear who is acceptable as an approver, and there should be at least one such person approving this PR.
• I like convenient defaults

GitHub tends to be rather eager to add reviewers, which means that by default PRs have all possible owners for all touched files. To some extent, we clean those up (I try hard not to have 10+ reviewers on cross-cutting PRs, which I make often).

I think there are two ways of looking at this, for PRs coming from ICU4X team members. Either the list of reviewers is a matter of PR author intent, or it is a matter of codeowner intent. Our policy is to require a single codeowner approval (per component), not multiple. This means, for PRs that do not nontrivially span multiple components, multiple reviewers may only be required as a matter of PR author intent. Put differently: codeowners should be okay with not approving every PR that touches their code: it is fine for a codeowner to mark themselves as a blocker on specific PRs (we've all done this at times), but policy for all components should remain that it requires a single codeowner approval, and anything on top of that is a matter of PR author intent.

For PRs coming from non-team members, the "author intent" switches to "merging team member's intent" but it's much the same.

With that established, I think that the question basically boils down to whether we feel it is important for the PR author to express their intent on the PR reviewer list when making the PR, or if they can express it when they hit merge. I'll point out that given how eager GitHub is to add reviewers, if I were to follow policy the actual list of events that would probably happen is:

• I open a PR with the CLI, specifying some explicit reviewers, GitHub adds extra reviewers
• The PR gets approved by one person
• I realize there were extra reviewers, and remove them, and hit merge

which is basically identical to what happens now except I do a seemingly-pointless bit of button clicking right before I merge.

I think having the reviewer list being treated by default as an ANY list is valuable: this is almost always what I want from my reviewers, especially with the timezones involved. I like having this ability and do not wish to lose it. I think having it as an ALL list does not particularly signal anything useful: it does not seem common enough to need to do, and in the uncommon cases it feels clearer to specify it in the PR body with detail.

[Manishearth]

cc @hsivonen @zbraniecki @robertbastian @aethanyc @nekevss @echeran @makotokato (all active CODEOWNERS)

[sffc]

Axioms I hold, part 1:

• ICU4X architecture can be difficult to understand, even for core team members.
• CODEOWNERS reflect, or should reflect, the team member with the strongest mental model of how a certain component works. This is usually the author, but not always (such as with internships).
• PRs that are not written or approved by someone with a strong mental model of a component could have unintended consequences. (I can dig up examples of this.)

This leads to my long-held position that either the author of a PR or at least one approver of a PR should be a CODEOWNER of each component that is receiving more than trivial changes.

Axioms I hold, part 2:

• Reviewers should be able to look at their list of outstanding reviews and know what solemn work has been requested of them from solemn PR authors.
• Merged PRs with pending reviews are extremely easy to overlook as the reviewer. There is no expectation that a pending reviewer will ever see the code.
  • Anecdote: I have 445 closed or merged PRs that are pending my review, but I have no easy way to tell how many of those actually wanted my review.
• The time zone gradient causes certain reviewers to be the first reviewer more than others.
  • Anecdote: I feel that I am personally the largest casualty of noncompliance with the current policy. I am seldom the fastest reviewer, for various reasons, and this puts me at a unique disadvantage for being able to review code that is landing in components where I have the strongest mental model, such as icu_datetime.
• The ICU4X project embraces GitHub tooling, including its deficiencies, and working around its deficiencies may require manual labor and social conformance. We do not use other tools like rfc-bot to manage review queues.

This leads to my position that there exists a duty to ensure that a PR's reviewer list in GitHub correctly reflects whatever policy we agree on.

Axioms I hold, part 3:

• Opening a Pull Request is a solemn duty.
• The author of a Pull Request is in the best position to know what components are touched in nontrivial ways. Reviewers are much more likely to overlook important things.
• The GitHub UI is the only officially supported method for opening Pull Requests to ICU4X and other Unicode projects. Other tools such as the GitHub CLI are available for convenience only.

This leads to my long-held position that as a project we can place certain responsibilities upon the author of a Pull Request, even if it requires a minute of manual labor not available via convenience tools such as the GitHub CLI. We should not rely on the "fastest reviewer" to make any judgements that might need to be made.

[Manishearth]

Thanks for splitting up the axioms, this is useful, and points to some cruxes of the discussion.

Apologies in advance for the multiple prongs of discussion presented below, but I think they actually do end up identifying some useful underlying points of contentions. I've listed them at the bottom.

Terminology note: here I use "ANY review" to mean "a review request of the form any(reviewer1, reviewer2, ...)

Axioms I hold, part 1:

I agree with this mostly, especially the part about the strong mental model. We should get better at requiring reviews from someone with a strong mental model.

I agree that the PR must be approved by a codeowner.

CODEOWNERS reflect, or should reflect, the team member with the strongest mental model of how a certain component works. This is usually the author, but not always (such as with internships).

I think we should perhaps dig into this. We have two owners per component right now, however a simple reading of this statement indicates that it should be just one.

I think we do indeed have components where there is only one person with that mental model (e.g. @sffc for datetime). I also think we have a fair number of components where it's not too important: e.g. decimal is a bit of a community property at this point. The person with the best mental model of datagen has shifted back and forth.

I think a good model here is to have a certain bar of strength of mental model, rather than going for the strongest, and anyone who passes that is sufficient.

There's also a question about what this means for components like segmenter: the primary authors of that crate are not super active reviewers. I would of course want their review for changing an algorithm, but what about a PR like https://github.com/unicode-org/icu4x/pull/6409 that rearranges the API?

So there's some nuance here.

Reviewers should be able to look at their list of outstanding reviews and know what solemn work has been requested of them from solemn PR authors.

@sffc The word "solemn" here does not appear to carry much meaning here, but clearly you intend it to, please be more explicit.

To directly address this point: elsewhere @sffc suggested that when I wish for an ANY review, I should tag icu4x-owners as a reviewer. Does that not still violate this axiom? (It's fine if it does, I'm just trying to understand)

Anecdote: I have 445 closed or merged PRs that are pending my review, but I have no easy way to tell how many of those actually wanted my review.

Given that we have a strong policy that PR authors merge their own PRs (except for trivial, autosumbit, or contributor cases), does that not naturally follow from the PR being merged: your review is not wanted anymore, it is just a cherry on top if you choose to provide it?

Would it be acceptable if people who requested multiple reviews but only needed one were asked to untag reviewers after merge? Trying to understand if the main contention here is about the closed PR list.

A question I have for the group is: how often is it that you actually want multiple reviews for a crate, but wish to merge something on first review? Would it be acceptable in that case to be more explicit in pinging the second reviewer? I've done this on occasion; merged something and then DMd the person to say "you should double check this eventually".

The GitHub UI is the only officially supported method for opening Pull Requests to ICU4X and other Unicode projects. Other tools such as the GitHub CLI are available for convenience only.

To be clear, I'm not particularly worried with having to do something on the website after using the CLI. I mentioned the CLI to faithfully reproduce my workflow. I usually end up going to the website anyway, e.g. to remove spurious reviewers for trivial changes to other code.

The time zone gradient causes certain reviewers to be the first reviewer more than others.

In a world where we are diligent about marking only our preferred reviewers on our PRs, will this not naturally end up with a situation where people, wishing for their PRs to land faster, will simply choose faster reviewers? I already choose in-timezone reviewers by default, most of my PRs go to @sffc when there is a choice between @sffc and @robertbastian , unless I am writing the PR in the evening.

I think this points to the crux of the discussion, as I highlighted before, there are two ways of looking at this: what the PR author intends, and what the codeowner intends. If we are talking solely about PR author intent here, it doesn't matter too much if one reviewer ends up reviewing more often from an ANY set. Instead, the problem is about PR author intent not correctly factoring in the "get all reviews from codeowners" requirement.

I think this also ties into the dual purpose of code review. One purpose is to produce better code, the other is to keep owners apprised of what is happening in their component. This is possibly a source of "PR author intent"

In general I would like us to be in a world where codeowners can trust team members to not land things that need their review. I'm not a fan of a setup where we have process replacing such trust: I think we should make our CODEOWNERS file reflect reality, expand the policy to talk more about when we require owner review[^1], and try to ask everyone to be more diligent there. Then, PR author intent is sufficient in capturing everything: the diligent PR author merges when they feel that all the required reviews are done. I think we have not been as diligent here, and it might be worth having work there.

Overall I think the ability to request an ANY review is common and desired, and not having that is onerous beyond just the deficiencies in GitHub's defaults. I think it is suboptimal to have PR authors pick a particular reviewer each time they need an ANY review.

This leads to my long-held position that as a project we can place certain responsibilities upon the author of a Pull Request, even if it requires a minute of manual labor not available via convenience tools such as the GitHub CLI.

To highlight this: my position is not about convenience, I'm disappointed with but not strongly against requiring additional button-clicking. However, I wish to retain the ability to request ANY reviews, and consider it super valuable. I think that is where the disagreement lies.

We should not rely on the "fastest reviewer" to make any judgements that might need to be made.

And I think we should be able to trust the reviewer to be able to call out things that are important enough to need further eyes, such that it is irrelevant that they were the first reviewer. I do not think we are currently in that state of trust, and I think it is important for us to fix that as a project, potentially by noting when this fails and trying to understand each others' priorities / review strategies when that happens. Reviewers should understand what other reviewers value when they share a component.

To me, that is a bigger underlying problem here, that is not solved by further process.

---

I do think a part of the issue here is that, specifically around datetime, @sffc is the only real codeowner of that crate at this point; whereas I've done extensive review but am not actually a codeowner. Potentially we should be more diligent for that crate in particular.

---

TLDR

• We should figure out what our bar for CODEOWNER is, because I don't think we are on the same page there as a group, and not due to any disagreement, just because it's not really been talked about.
• We should figure out what exactly is the bar for needing codeowner review for a PR, because it's more nuanced than "everything but LSCs"
• When a PR author desires an ANY review for a single component change, should they:
  • Request an ALL review, they are incorrect for wanting an ANY review, all codeowners should approve
  • Pick someone and request their review.
  • Request icu4x-owners and cc their favored requestees?
  • Add all reviewers, potentially removing ones after the first reviewer looks at it.
• We need to better specify the push-pull dynamic of when someone blocks a PR. Should it be based on PR author's intent (with reviewers being able to override / insert themselves into the process?), or should all codeowners block PRs by default?
• We should talk about building a system based in trust, and what we need to do to get there.

[^1]: There's a lot of edge cases. For example, obviously a PR written by the sole CODEOWNER can go to anyone. Perhaps things like surface level API organization can also go to anyone, or an icu4x-owner. I feel like the current policy has too many edge cases to be useful.

[Manishearth]

The comment above identifies a bunch of things which really look like prerequisite discussions, which we perhaps should have first before talking about this policy, specifically the discussions about what codeowners means, when a codeowner should be required, and how to improve cross-reviewer trust.

(I do think a fair amount of our issues here will be solved by having a codeowner policy that handles enough edge cases that it is not easy to just ignore it because it feels like an edge case)

[hsivonen]

• The time zone gradient causes certain reviewers to be the first reviewer more than others.

I think the time scale issue is important, and it isn't just about the time zone gradient.

For making progress and not having to rebase a queue of bitrotting patches, it's important that reviewers responds relatively quickly. I think I've been on both sides of CODEOWNER taking more than a couple of days to review when other icu4x-owners have been around to do same-day reviews.

[sffc]

So there's some nuance here.

Sure, and I didn't mean to say that there must only be 1 codeowner. I agree that not all codeowners have the same expectations as others, but the system doesn't really allow for that. Ideally there would be "real codeowner" (could even include interns) and "effective codeowner" (the more active team member).

To directly address this point: elsewhere @sffc suggested that when I wish for an ANY review, I should tag icu4x-owners as a reviewer. Does that not still violate this axiom? (It's fine if it does, I'm just trying to understand)

GitHub UI allows me to select reviews for "me" or "me and my team". This is the primary UI that I use to determine what review queue work I have cut out for me on a given day. I primarily try to get through the "me" queue and also try to be a good citizen and pull things from the "me and my team" queue.

Would it be acceptable if people who requested multiple reviews but only needed one were asked to untag reviewers after merge? Trying to understand if the main contention here is about the closed PR list.

Yes, if my post-submit review is not requested, due to codeowners requirements having been met, you may remove my review request, but ideally you never asked for it in the first place.

A question I have for the group is: how often is it that you actually want multiple reviews for a crate, but wish to merge something on first review?

I have an expectation that given time zone gradients, things that are "easy to undo" are merged fairly frequently, with an expectation that someone (usually me, a slower-than-average reviewer) will come along later and add a post-submit review. This is understandable, but it only works if we keep the post-submit review queue clean and manageable.

In other words, this is a case that I expect to be the norm, not an exception. You want to get unblocked so you get a non-codeowner approval on something easy to undo (naming, etc) while you wait for the codeowner review, which might take days to weeks to come in.

Things that are harder to undo should still wait for codeowner approval.

Would it be acceptable in that case to be more explicit in pinging the second reviewer? I've done this on occasion; merged something and then DMd the person to say "you should double check this eventually".

I would really like to avoid relying on DMs for things.

However, I wish to retain the ability to request ANY reviews, and consider it super valuable. I think that is where the disagreement lies.

I agree with wanting the ability to request a generic ("ANY") review!

To me, that is a bigger underlying problem here, that is not solved by further process.

We should start by being more clear about the intent of things. Semantic Comments (Issue/Question/Nit/Suggestion) have been extremely helpful at conveying intent, and we need to translate this clear communication precedent into our review queue process. For example, I have no way to read the intent of a PR with no description and a vague title that touches a file in icu_datetime and has me set as a reviewer. If the review is not a codeowner thing and just needs a generic approval (LSC, previously-approved rename, etc), having a generic reviewer (I keep suggesting @unicode-org/icu4x-owners) conveys that intent more clearly.