1  The purpose of code review

The primary purpose of code review is to ensure that the overall “code health” of our packages is improving over time. In order to accomplish this, a series of trade-offs have to be balanced.

First, developers must be able to make progress on their tasks. If a reviewer makes it very difficult for any change to go in, then authors are discouraged from making improvements in the future.

On the other hand, it is the duty of the reviewer to make sure that each PR is of such a quality that the overall code health of the package is not decreasing as time goes on. This can be tricky, because codebases often degrade through small decreases in code health over time. This isn’t necessarily anyone’s fault. It is a natural process resulting from the accumulation of technical debt over time, which must continually be fought against.

Additionally, a reviewer often has ownership and responsibility over the code they are reviewing. When the reviewer is also the maintainer of the software, keeping the codebase consistent and maintainable is an even higher priority, because the reviewer is the one that has to maintain the code going forward.

With those tradeoffs in mind, we get the following rule as the standard we expect of code reviews:

In general, reviewers should favor approving a PR once it is in a state where it definitely improves the overall code health of the system being worked on, even if the PR isn’t perfect.

A key point here is that there is no such thing as “perfect” code—there is only better code. Reviewers should not require the author to polish every tiny piece of a PR before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting. Instead of seeking perfection, what a reviewer should seek is continuous improvement. A PR that, as a whole, improves the maintainability, readability, and understandability of the package shouldn’t be delayed for days or weeks because it isn’t “perfect.”

Reviewers should always feel free to leave comments expressing that something could be better, but if it’s not very important, note in the comment that this is just a point of polish that the author can choose to ignore.

1.1 Mentoring

At Posit, we believe that code review is an important part of on-boarding new developers. As a reviewer, you are encouraged to leave comments that help an author learn something new. Linking out to sections of the style guide, design guide, or this code review guide are ways to point to a concrete source of truth that the PR author can read to learn how to align their contributions with tidyteam standards.

Pair programming is also an excellent way to perform a code review. One way to do this is to have the reviewer live-review a pull request, which often teaches the author what the reviewer looks for when they review PRs, and gives both parties a way to have an immediate discussion about any tricky design points. It is generally useful to document any insights from the pair programming session as a comment in the code review.