About
Policies and Security
Product
Bounty Programs
Guidelines
<aside> <img src="https://s3-us-west-2.amazonaws.com/secure.notion-static.com/e99d759d-b386-4668-b749-afbe05c3bada/Machi_M_280x280.png" alt="https://s3-us-west-2.amazonaws.com/secure.notion-static.com/e99d759d-b386-4668-b749-afbe05c3bada/Machi_M_280x280.png" width="40px" /> These guidelines document how to review code. Helpful for new and remote contributors to get and stay aligned.
</aside>
A code review is a process where someone other than the author(s) of a piece of code examines that code.
At Machi-Systems, we use code reviews to maintain the quality of our code and products.
This documentation is the canonical description of Machi-Systems code review processes and policies.
This page is an overview of our code review process.
Why do you perform code reviews? What are your guiding principles for these reviews?
You may want to mention other pages here. Like Engineering Guidelines. To link to another page inline, type @ followed by the name of the page: ‣
Code reviews should look at:
See How To Do A Code Review for more information.
In general, you want to find the best reviewers you can who are capable of responding to your review within a reasonable period of time.
The best reviewer is the person who will be able to give you the most thorough and correct review for the piece of code you are writing. This usually means the owner(s) of the code, who may or may not be the people in the OWNERS file. Sometimes this means asking different people to review different parts of the Change-list (CL).
Preparation sets your reviewers up for success.
The sections contain recommendations on the best way to do code reviews, based on long experience. All together they represent one complete document, broken up into many separate sections. You don’t have to read them all, but many people have found it very helpful to themselves and their team to read the entire set.
See also the CL Author’s Guide, which gives detailed guidance to developers whose CLs are undergoing review.
The primary purpose of code review is to make sure that the overall code health of Machi-Systems’ code base is improving over time. All of the tools and processes of code review are designed to this end.
In order to accomplish this, a series of trade-offs have to be balanced.
First, developers must be able to make progress on their tasks. If you never submit an improvement to the codebase, then the codebase never improves. Also, if a reviewer makes it very difficult for any change to go in, then developers are disincentivized to make improvements in the future.
On the other hand, it is the duty of the reviewer to make sure that each CL is of such a quality that the overall code health of their codebase is not decreasing as time goes on. This can be tricky, because often, codebases degrade through small decreases in code health over time, especially when a team is under significant time constraints and they feel that they have to take shortcuts in order to accomplish their goals.
Also, a reviewer has ownership and responsibility over the code they are reviewing. They want to ensure that the codebase stays consistent, maintainable, and all of the other things mentioned in “What to look for in a code review.”
Thus, we get the following rule as the standard we expect in code reviews:
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.
That is the senior principle among all of the code review guidelines.
There are limitations to this, of course. For example, if a CL adds a feature that the reviewer doesn’t want in their system, then the reviewer can certainly deny approval even if the code is well-designed.
A key point here is that there is no such thing as “perfect” code—there is only better code. Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting. Instead of seeking perfection, what a reviewer should seek is continuous improvement. A CL that, as a whole, improves the maintainability, readability, and understandability of the system shouldn’t be delayed for days or weeks because it isn’t “perfect.”
Reviewers should always feel free to leave comments expressing that something could be better, but if it’s not very important, prefix it with something like “Nit: “ to let the author know that it’s just a point of polish that they could choose to ignore.
Note: Nothing in this document justifies checking in CLs that definitely worsen the overall code health of the system. The only time you would do that would be in an emergency.
Sometimes there are emergency CLs that must pass through the entire code review process as quickly as possible.
An emergency CL would be a small change that: allows a major launch to continue instead of rolling back, fixes a bug significantly affecting users in production, handles a pressing legal issue, closes a major security hole, etc.
In emergencies we really do care about the speed of the entire code review process, not just the speed of response. In this case only, the reviewer should care more about the speed of the review and the correctness of the code (does it actually resolve the emergency?) than anything else. Also (perhaps obviously) such reviews should take priority over all other code reviews, when they come up.
However, after the emergency is resolved you should look over the emergency CLs again and give them a more thorough review.
To be clear, the following cases are not an emergency:
..And so on.
A hard deadline is one where something disastrous would happen if you miss it. For example:
