Table of Contents

This post is about reviewing Pull Requests (PRs) on a software engineering team, often submitted in a source code system like Github. It’s meant to be a concise guide.

Goals of a PR Review

For many PRs, you don’t need to be an expert in the stack or programming language that is used in the PR.

The time it takes to review a PR is directly related to how long the PR took to create and how many changes it includes. You should expect to spend more time reviewing a large PR than a small PR.

Required Steps When Reviewing

The steps below apply to PRs with changes to code. If it’s a non-code PR (e.g. a wiki update), the reviewer should just check for typos and that the changes make sense to them.

  1. Confirm that the PR description contains at least 1-2 items describing the changes being made and the reason for those changes.
  2. If there aren’t automated tests, confirm that the PR description contains at least 1-2 items describing how the changes were tested.
  3. Check each changed file for potential bugs that the submitter may have missed.
  4. Check to see if __the code is readable__.
  5. Check for typos in the comments and readmes.
  6. If you’re not proficient with the programming language or framework used, make a note of that in your review.
    1. For example, “I'm a beginner when it comes to JavaScript. This PR looks good to me, but if you have any doubts about your code or would like a more in-depth review, please consider requesting a second review in ___ channel.”
    2. You don't have to be an expert in the system which the PR is changing, but you should have a basic understanding of what's going on. If you have absolutely no clue, it's best to either loop in someone who does, suggest that the submitter bump their request for a review, or ask for a review in another channel.
  1. If there are any parts that are particularly difficult to understand, consider suggesting that the submitter refactor or add a comment.

Example

🎉 Lgtm!

My gitlab CI configuration experience level: beginner.

I did not see any obvious errors, everything looks readable and elegant.

If at all possible, try to make future PRs smaller. If not possible, please note that in the PR description.

ci/scripts/pipelines.py

It looks like there are some major changes to the gitlab pipeline in this PR. I reviewed the testing methodology, and it looks pretty good, but due to the size of this PR, please take a second look at the tests you did to make sure these changes wont break anything before merging the PR.

Great work 🔥. Thank you for making these gitlab CI improvements!