Skip to content

Advice for Reviewing Pull Requests

Jump to bottom
mnatsuhara edited this page Jun 17, 2020 · 1 revision

Suggested Strategy

  • Begin by reading through the pull request's description and at least skimming any linked issues, proposals, etc. to gain some context for the changes you will review.
  • Some find it helpful to also read through the existing conversation that has already occurred in that pull request.
    • This can help you avoid commenting on things that have already been noted by another reviewer but have not yet been fixed.
    • If you are unfamiliar with the files being changed, preexisting conversation can help you become more familiar with the subtleties involved
  • Utilize GitHub's "reviews" feature that allows you to batch up your comments and do a complete review before submitting them.
    • Using this allows you to comment on things as you go, but you can always go back and edit them later if you make new discoveries while continuing to review the changes made.
    • This feature also allows you to treat review comments as notes for you to refer back to as you look through the files.
  • You may find doing multiple passes over the changes made to be helpful; for example, you could do a first pass to review the larger, structural changes made, then another pass to nitpick over comments and naming conventions.
  • Start with product code changes (before testing code).
    • Sometimes you will notice unusual things or handling of edge cases in product code, and you can later ensure that these cases are tested when you move on to reviewing testing code.
    • Some prefer to start with smaller, independent changes before moving on to monster file changes.
  • In reviewing a pull request implementing a new feature, take time to review the proposal for the feature and the Standard to ensure that all components are present and all elements are implemented.

Additional tips/tricks

  • Use GitHub's "Viewed" feature to keep track of which files you have already reviewed (this is particularly helpful in large pull requests involving changes to many files).
  • When there are repetitive changes, try to verify that the repetition is exact, the context remains applicable in each case (e.g., things are still lvalues or rvalues), and there are no missing occurrences (especially across files).
  • Control flow alteration is a particularly risky change. Pay careful attention here to ensure that loops, branches, etc. will behave the same way, particularly in the case of boundary conditions.
  • Metaprogramming entails lots of corner cases, especially because (pre-concepts) there is no meta-type system to detect mistakes. Here, it is a good idea to consider questions like "What if this is const?", "What if this is a reference?", "What if this is a function type?".
Clone this wiki locally