3.1 Summary
Now that you know what to look for, what’s the most efficient way to manage a review that’s spread across multiple files? Here are three steps to tackle most code reviews:
- Before looking at any code, ask yourself if the rationale behind the change make sense.
- Look at the most important part of the change first. Is it well-designed overall?
- Look at the rest of the PR in an appropriate sequence.
3.2 Take a broad view of the change
Before looking at any code, you should familiarize yourself with the GitHub issue / bug report that the PR is resolving, and read over the PR description to better orient yourself. Does this change even make sense? You can save everyone time if the answer is no by stopping the review there and responding with an explanation of why the change isn’t necessary. Ideally, when performing close-knit collaboration, you will have discussed this ahead of time to avoid the author putting in unnecessary work (and feeling bad if their work is rejected). For external contributions, occasionally you do have to reject PRs if they don’t fit the overall design of the package. You should do this nicely, thanking them for their contribution, and requesting that next time they open an issue first and ask if you’d accept a PR for that issue, which would have saved them time if you had an opportunity to preemptively say no.
For example, you might say: “Looks like you put some good work into this, thanks! However, we’re actually going in the direction of removing the foo() helper that you’re modifying here, and so we don’t want to make any new modifications to it right now. In the future, it is best if you open an issue first to highlight the bug or feature and ask if we’d accept a pull request for it at this time.”
If you get more than a few PRs that represent changes you don’t want to make, you should consider re-working your team’s development process or the posted process for external contributors so that there is more communication before PRs are written. It’s better to tell people “no” before they’ve done a ton of work that now has to be thrown away or drastically re-written.
3.3 Examine the main parts of the PR
Find the file or files that are the “main” part of this PR. Often, there is one file that has the largest number of logical changes. Ideally, the PR description will point you to this if there is any ambiguity. Look at these major parts first. This helps give context to all of the smaller parts of the PR, and generally accelerates doing the code review. If the PR is too large for you to figure out which parts are the major parts, ask the author to provide a recommended reading order, or ask them to split up the PR into multiple focused PRs.
If you see some major design problems with this part of the PR, you should send those comments immediately, even if you don’t have time to review the rest of the PR right now. In fact, reviewing the rest of the PR might be a waste of time, because if the design problems are significant enough, a lot of the other code under review is going to disappear and not matter anyway.
This is also a great time to use usethis::pr_fetch() to pull the PR down locally and try it out. Chances are that the main part of the PR has to do with the main UI changes, so this is a nice time to check that the changes in the PR line up with the output you get from running the code locally.
3.4 Look through the rest of the PR
Once you’ve confirmed there are no major design problems with the PR as a whole, try to figure out a logical sequence to look through the files while also making sure you don’t miss reviewing any file. Usually after you’ve looked through the major files, it’s simplest to just go through each file in the order that the code review tool presents them to you. Sometimes it’s also helpful to read the tests first before you read the main code, because then you have an idea of what the change is supposed to be doing.
For R code, you can generally ignore the generated documentation files ending in .Rd while reviewing the PR. GitHub will collapse these by default.
When you’ve finished your review, if you’ve checked out the PR locally you can remove it with usethis::pr_forget().