Code reviews

All code should be reviewed and approved by an appropriate teammate before being merged into the master branch.

Our goal is to have a code review process and culture that everyone would opt-in to even if code reviews weren’t required.

Why do we require peer code reviews?

Code reviews are an investment. As an author, it takes extra time and effort to create good PRs, wait for someone to review your code, and address review comments. As a code reviewer, it takes extra time to understand the motivation, design, and implementation of someone else’s code so that you can provide valuable feedback. This investment is worth it because it provides benefits to every person on the team and it helps the team as a whole ship features to our customers that are high quality and maintainable.

For authors:

  • Requesting a code review from one of your peers is motivation to ensure that the quality of your code is high.
  • Writing a good PR description (and commit messages) develops your technical communication skills.
  • Receiving code review feedback can give you valuable insight into aspects of your change that you hadn’t considered (architectural, performance, etc).
  • Receiving feedback about how your code could be improved helps you learn how to write better code in the future.

For reviewers:

  • Reviewing code from others increases your code literacy and empathy. You will learn new things from how your peers write code and you will be more aware of how to write code that is easier for your reviewers to understand
  • Reading PR descriptions increases your technical literacy and empathy. You will learn what makes a PR description effective and you will be more aware of how to write PR descriptions that are easier for your reviewers to understand.
  • Being a reviewer gives you the ability to share knowledge you have that others do not. Every person, regardless of experience level, has both something to learn and something to contribute.

For the team:

  • Code reviews increase the overall quality of our codebase by catching design and implementation defects before changes ship to customers.
  • Code reviews increase the overall consistency of our codebase by spreading knowledge of best practices and eliminating anti-patterns.
  • Code reviews distribute domain expertise, code knowledge, and ownership across the team so development can continue when people are unavailable (e.g. vacation).

When is a code review not required?

We do not technically prevent PRs from being merged before being explicitly approved because there exist PRs that are appropriate to merge without waiting for a review.

Some examples:

  • Reverting a previous change to solve a production issue.
  • Minor documentation changes.
  • Auto-generated changes, such as when performing a version release, where a reviewer would not have anything of substance to review.

It should be obvious to any other engineer on the team from the PR description and/or diff that it was appropriate for you to not wait for approval.

Here are some examples of reasons to skip code review that are NOT acceptable:

  • “I promised that I would ship this to $CUSTOMER by $DEADLINE”
    • The customer expects the feature to work and be maintained. Code review helps ensure both of these things by increasing the quality and distributing ownership.
  • “This code is experimental”
    • Our goal is to have a code review culture such that engineers who are working on “experimental” code still find code reviews valuable and worth doing (for all the benefits mentioned in the rest of this document).
    • All code that is in master has the potential to impact customers (e.g. by causing a bug) and other developers at Sourcegraph (e.g. by making it harder to refactor code). As such, it is in our interest to ensure a certain quality level on all code whether or not it is considered “experimental”.
    • Assume that we allowed “experimental” code to bypass code review. How would we know when it is no longer experimental and how would it get reviewed? Either it wouldn’t get reviewed, or an engineer would have to review all the code after the fact without a nice PR diff to look at or effective way to make comments. Neither of these outcomes would meet our need of reviewing all non-experimental code.
  • “I don’t have someone to review this code”
    • Ask for help to identify someone else on the team with whom you can share your knowledge, context, and ownership.

If we see that there are too many PRs being merged without approval that lack an appropriate reason, then we will have to consider an automated solution to enforce code reviews.

What makes an effective Pull Request (PR)?

An effective PR minimizes the amount of effort that is required for the reviewer to understand your change so that they can provide high quality feedback in a timely manner. Futher reading.

Do:

  • Prefer small PRs. If a large PR is unavoidable, try to organize it into smaller commits (with good commit messages) that are reviewable independently (and indicate this to the reviewer in the description).
  • Create a draft PR first and review your own diff as if you were reviewing someone else’s change. This helps you empathize with your reviewer and can help catch silly mistakes before your reviewer sees them (e.g. forgetting to git add a file, forgetting to remove debugging code, etc.).
  • Create meaningful PR title and description that communicates what the PR does and why (the how is the diff).
    • Include links to relevant issues (e.g. “closes #1234”, “helps #5678”).
    • Include a screenshot, GIF, or video if you are making user facing changes.
    • Discuss alternative implementations that you considered but didn’t pursue (and why).
    • Describe any remaining open questions that you are seeking feedback on.
  • Make it clear who’s review(s) you are waiting for.
  • Politely remind your reviewer that you are waiting for a review if they haven’t responded after one business day.

What makes an effective code review?

This blog post effectively explains some principals which our team values.

The code author and code reviewer have a shared goal to bring the PR into a state that meets our quality standards and the needs that motivated the change.

Do:

  • Take time to understand the context and goal of the PR. If it isn’t clear, ask for clarification.

  • Acknowledge what the author did well to balance the tone of the review.

  • Make it clear which comments are blocking your explicit approval (e.g. use “nit:” prefix for minor comments) and approve if all of your comments are minor.

  • If the author were to address all of your comments faithfully and you would be content, then you should also approve to avoid the author needing to wait for a subsequent review without reason (exception: you asked for fundamental or vast/large changes and believe those will need re-review by you).

  • When you are making comments on a PR, use a tone that is kind, empathetic, collaborative, and humble. Further reading.

    😕 🤗 Why?
    You misspelled “successfully” sucessfully -> successfully Avoid using “you” because it can make the comment feel like a personal attack instead of just a minor correction.
    Move the Foo class to a separate file. Can we move the Foo class to a separate file to avoid large files that are hard to scan? Feedback that is framed as a request encourages collaborative conversation. Including your reason helps the author understand the motivation behind your request.
    Can we simplify this with a list comprehension? Consider simplifying with a list comprehension like this: $EXAMPLE or $LINK_TO_EXAMPLE Providing example code (using GitHub suggestions) or a link to an example helps the author quickly understand and apply your recommended change. This is particularly helpful when the author might not be familiar with concept that you are describing.
    Why didn’t you $X? What do you think about doing $X? I think it would help with $Y. A “What” question is better than a “Why” question because the latter sounds accusatory. Either the author considered $X and decided not to, or they didn’t consider it, but in either case it is frustrating to the author if the reviewer doesn’t explain why they think $X should be considered.
    This code is confusing. Can you clarify it? It was hard for me to understand $X because $Y, but I haven’t been able to think of a way to improve it. Do you have any ideas? Don’t declare your opinion as fact; instead, share your subjective experience and try to offer a suggestion for improvement. If you don’t have a suggestion, acknowledge that to show that you empathize with the difficulty of improving this code.

Don’t:

  • Comment on the details of the PR if you have questions or concerns about the overall direction or design. The former will distract from the latter and might be irrelevant if the PR is reworked.
  • Take longer than one business day to respond to a PR that is ready for your review (or re-review).
  • Have protracted discussions in PR comments. If it can’t be settled quickly in a few review round trips, try discussing in person or on a video call because these mediums are higher bandwidth and encourage empathy. Then summarize the results in the PR discussion after the fact.

Who should I get a code review from?

You should get a code review from the person who’s approval will give you the most confidence that your change is high quality. If you are modifying existing code, you can use git blame to identify the previous author and the previous reviewer because they probably will have helpful context.

If your change touches multiple parts of our codebase (e.g. Go, TypeScript), then you will need to get approval from multiple peers (e.g. a Go reviewer and a TypeScript reviewer).

GitHub will automatically assign reviewers if there is a matching entry in the CODEOWNERS file, but that doesn’t necessarily mean that you need to wait for an approval from everyone. For example, if you are making a change to the search backend then you only need approval from one person on that team, not all of them.

Use your best judgement and ask if you are uncertain.