All code should be reviewed and approved by an appropriate teammate before being merged into the
Our goal is to have a code review process and culture that everyone would opt-in to even if code reviews weren’t required.
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 the team:
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.
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:
masterhas 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”.
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.
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.
git adda file, forgetting to remove debugging code, etc.).
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.
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.
|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.|
||Can we move the
||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.|
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.