Reviewing PRs

[clyne]

It seems like we've tripped up a few times getting reviewers assigned to PRs, and then getting PRs reviewed. I wanted to share what we do with one of my other projects that seems to work pretty well for us, and see if people thought this might be helpful for Pythia. The basic goal is to make sure that PRs are getting reviewed by the "right" people, and done so in a reasonably timely manner. So here goes:

For each repo we assign a "repo manager" (this could also be done based on PR type, currently "infrastructure" or "content" in our case). The basic job of the repo manager is to assign reviewers to PRs in a timely manner. The repo manager can use their judgment in selecting how many reviewers and who. For simple changes (e.g. fixing a typo) the repo manager may just assign themself. If an assigned reviewer doesn't think they are appropriate for the PR, they can request to be removed, and the repo manager can find someone else. Moreover the PR submitter can suggest reviewers when the PR is submitted.

It's also the repo managers job to crack the whip if needed, nudging reviewers to get their reviews in, and possibly reassigning if needed. Or if a PR is getting bogged down in endless discussion, the repo manager may need to step in to try to find a compromise.

Lastly, only repo manager is allowed to merge the PR after all of the assigned reviewers have approved.

As I said this process has worked pretty well for more software development focused projects. Just thought I'd toss this out there for consideration.

[brian-rose]

John, this sounds like a reasonable system to me, with the repo manager playing a role very analogous to a journal editor. I can see how it would speed up our development to have a system in place for thoughtful and specific reviewer assignments.

One issue that I can foresee arising is that the repo manager (as the traffic cop of the whole process) becomes a critical bottleneck. On your other projects, do you typically have an alternate repo manager to step in when the primary person needs to step away (due to vacation time or other overwhelming obligations)?

[dopplershift]

I think the repo manager is a good way to help drive the process forward, but only as a method of (for lack of a better word) accountability, not any kind of gate-keeping step. So in that regards, I don't see their temporary absence as a bottleneck, just a lack of "oil in the machinery."

This is an open project with a desire to be a community project. As such, it's perfectly fine for any of us with write access (or in CODEOWNERS) to review a PR and (gasp) click the green button to merge. I think the lack of throughput is partially because we're in the early stages and people don't feel the ownership or agency necessary to do it (just a guess). Plus with not seeing many go through, it's unclear what a "good review" looks like.

Personally, I'll be trying to step up and review some once we have some more PRs that aren't based on content I wrote. 😆

[clyne]

We don't have an alternate, as there has never been a need. But there is no reason you couldn't have one. The main task of the repo is to make sure reviewers are assigned and to keep things moving forward.