Dev Process: Formalizing how PR flow & PR State

[DanVanAtta]

I think it might be beneficial to take a swing at formalizing the PR process and refining how we do PRs. The desired outcome of this discussion is that we'll have thought through the process more, shared ideas, and will have content that can then go onto the 'contributing.md' document (or similar) to specify how we handle PRs in this project.

With that:

Who can review

IMO anyone

How to respond to review comments - case: code changed - reply with SHA

I think if there are any code changes in response to a comment, the code change commit should be posted as a response. This makes it very easy to see which changes address which comments and that the comment was addressed. The reviewer can then review the diff of the commit and then either add follow-up comments or mark that thread as resolved.

This also helps notify the reviewer of changes. Pushing commits to a PR does not send notifications, but comments on a PR does. Ergo, this will be sufficient to notify a reviewer to have another look.

Using labels to indicate review status

I think we could benefit from at least two labels:

• review started: this means a review is in-progress, perhaps some comments have been left, but the reviewer is still reviewing the full diff and more comments may land. Ideally we can have reviewers complete reviews in just one session, but that is not always the case. This label would help us know that the reviewer is still working on the PR and that more comments might land.

• first review done: this means a reviewer has done a first full pass on the initial diff and any further review work will be done in response to new changes.

If a first-review pass is done, and there is no follow up needed, and the PR is approved and merged, then we don't need to bother labelling. A 'first review done' label therefore implies that there are questions or points of clarification needed from the author.

PR state (?) - assignments and how to "pass the ball"

A point I don't think that works very well is assigning between author and reviewer. I wonder if labels could be a good way to do this? I wonder as well if we simply reply to comments with commit SHA's, if that would be enough.

Perhaps if an author has replied to everything, that will give a notification to the reviewer who can then know they should look at the PR again. If an author has only done a partial response, and is still working on the PR, then in that case they can give a quick comment like "I responded to a couple of the comments, and am still working on the others".

In essence, maybe the way to pass the ball is simply by adding a comment of any type.

Anthing else?

This is an open discussion, please bring up any observed pain points or ideas for further improvement. Happy to discuss the above and would like feedback on those suggestions!

[trevan]

For "Who can review", I think it has to be someone with commit privileges if the PR author doesn't have commit privilege. Having a someone without commit privilege reviewing code for someone else who doesn't have commit privilege seems like it might be a waste of time since neither of them can approve it and commit it.

I'm not sure if it is needed to paste the commit hash in the comment. I think the commit should be pushed before your comment and then in your comment, reference that you made a change. Github will show it in time order so you can see the commit that happened before the comment. Now, if you push multiple commits for different concerns, referencing the commit hash would be useful.

[DanVanAtta]

Let's take this one topic at a time.

Who can review

We at one point had writtenas a guideline that reviewing a good chunk of code was a prerequisite for gaining write permission. There are a lot of examples of reviews coming from people that cannot commit. I certainly reviewed quite a bit of PRs before gaining write permission myself.

I think if we consider the objective and goals of CR (code review) it'll be pretty clear it's not a waste of time.

• peer review: This is arguably the biggest benefit and CR should (IMO) often be considered peer review. A peer is someone who has equal status in a project, that implies they not be one of the "main maintainers". The purpose of the peer review is to ensure that code reads well to another person who did not write it and does not bring the same level of context. This helps ensure that code is written well and clearly enough so that such a person can understand what has been developed. This is very important as we all lose context after a short time and this level of maintenance effort is persistent. For example, when tracking down a bug, or trying to understand what has been built so that it can be modified, all developers must repeat this process. If no code is reviewed and it's only easily understood by those who wrote it, when they wrote it, then the future effort to modify all such code is multiplied. This is perhaps the most important aspect, peer review is to ensure that a generic developer can come in and reasonably understand what is written without too much effort, they are verifying that the code is written in a relatively simple way by testing "can a person without too much context, such as myself, actually understand this easily?"
• project collaborations. Say two people are working on a feature together, certainly them reviewing each others work is useful.
• team scaling: having only maintainers review is putting an additional constraint on team personnel and capacity. It's already a scarce number of developers, restricting review to a subset of them further makes that labor even more scarce. What happens when the one or two people with write permission need to focus elsewhere in their life and are unavailable?
• knowledge sharing. Reviewing changes helps broadcast knowledge about how different parts of the code are built. This helps generalize the knowledge of the different modules so that we avoid having a sole maintainer of any one part of the code. If someone without write permission were to review lots of code, then when they do gain write permission, they'll have broad familiarity with the code base beyond the sections that they worked on themselves. This puts them in a good place to fix problems during release and to generally be a better reviewer themselves as they'll have more context of the broader codebase when they review other PRs submitted by others.
• Problem and bug spotting, it does not take a person with 'write' to be able to spot a logic flaw, or a missing test case.
• 3rd party library reviewers. We've had at least one example where the author of a third party library that we were adopting reviewed (without solicitation) our PR adopting their library. Their review was certainly useful and it pointed to a number of better ways to use that library.

So to sum up, the benefits of reviewing when you do not have write permissions:

• task sharing, offloads from maintainers
• code socialization and communication, builds a larger pool of potential maintainers
• is still useful, can spot design and code defects
• peer review, code should be easy to understand regardless of who you are and even particularly if you are new to the project.

[DanVanAtta]

Summary - I don't think this discussion was conclusive. With some time and reflection on CR processes, I do believe we should probably make them lighter/easier to finish. Open source and allowing a PR from anyone in the world, sometimes without any prior discussion, does make this a tricky subject. Overall I would like there to be less CR'ing and more merging (possibly directly) to trunk.