Code reviews are mandatory for every merge request. You should get familiar with and follow our code review values.
As reviewers, we want to strive to avoid degrading maintainability of the GitLab codebase, while empowering velocity as we build loveable features through a friendly and respectful environment.
We value the goals and objectives of all stakeholders of a merge request, but when the values of a stakeholder compete against another, we prioritize the following values:
Users > contributors > ourselves
It's important to keep in mind that reviewers/maintainers don't own the requirements. It's ultimately the job of Product/UX to create and uphold them. Please involve PM if there are concerns about the functionality being introduced in a contribution. However, we all play a part in keeping an eye on the holistic end-user experience.
At GitLab, we have a large selection of reviewers who strive for the highest quality when reviewing merge requests. However, due to human nature, we are subject to individual nuances.
This was raised as a discussion point in the Frontend maintainer meeting by @sarahghp as to "What is the ideal balance between code consistency team-to-team? Are we trying to pick too many nits in pursuit of consistency?", which evolved into a larger question:
As a reviewer, what values should we choose to enforce in reviews, and what can be considered with more flux? Following this question, with items granted more flux, how do we approach that? Do we aim for a collaborative model and encourage reviewers to share branches with authors or should we encourage more follow-up issues and merge requests? Or this is something that can be considered as a personal preference?
Simplifying:
Should we permit a merge request to be blocked when it is against our values?
Given these questions, we documented on this page a set of specific values to help everyone perform reviews consistently throughout the company.
In general, if a merge request is a net profit, then we should be eager to approve and move forward. This attitude takes into account things like:
The sum of all the pros and cons should lead to a net profit and the ability to move forward.
There are no solutions, only trade-offs. (Thomas Sowell)
In software, there are simply no perfect solutions. They do not exist. This is mostly due to the nature of computers (see Cap Theorem, for example). Also, the quality attributes are often at odds with each other. For example, enhancing security hurts performance, and some performance boosts are not maintainable. Software Engineering is about prioritization and balance.
The first and foremost step of being a great reviewer is learning to accept, manage, and communicate acceptable trade-offs in the codebase.
A merge request must have some thing to be considered “profitable.” Being a reviewer is a tough juggling act. You will find yourself often balancing iteration vs quality concerns, but a bias for quality serves you (and the rest of your teammates!) well. The following list includes a guide of some common (but not exhaustive) quality must-haves:
These must-haves should be the starting point for every code review when establishing permissions to play. Each reviewer has their own review style, and this is what makes them an individual reviewer. They have their own expertise to offer, but in general, we want to collectively commit to ensuring a high standard of quality at GitLab and service to our stakeholders.
Keep in mind not all changes are equally created. If the author is working on legacy code we shouldn't have as high-quality expectations as green-field code. The changes should still lead to a net gain in code quality, though.
When you are reviewing a merge request and identify a quality tradeoff, aim to communicate it well:
This helps to foster a bias for action and collaboration. Approach this with kindness and humility in your communication, as the author may not have realized that they introduced a difficult choice for the reviewer. They could just have tried to write good code to solve their problem. Often authors do not foresee the far-reaching effects of their choices (nor should they; this will lead to a lack of progress).
At GitLab, we iterate progressively and consider this is a core value for your judgment of a merge request's validity. We value iteration in GitLab because we value breaking problems down into their smallest pieces possible and because it calls for a bias for action.
We truly believe in this as a guiding principle that makes our jobs as reviewers easier, since the content is small and digestible. However, it's also important to be aware that multiple iterations do not always make things easier. Sometimes they introduce difficulty to grasp the context. However, these issues are outweighed by the benefits of high velocity, feedback, and reliability. This is something that we, as reviewers, should be constantly encouraging in authors. "Can we make this smaller?" should be a very common question to ask ourselves when starting a review.
It's very possible to take a lot of small confident steps to a mess. (Paul Slaughter)
The iterative approach also teaches authors to think out the solution to a large or complex problem by ensuring it's broken into small, quality-driven pieces.
Some practical (but not exhaustive) examples of quality to watch for as a reviewer:
Being honest can be hard. And even harder when there is a perceived imbalance in authority between the merge author and the merge reviewer:
This shouldn't happen, though. What we want to do here is always encourage a fearless sense of collaboration between the author and reviewer through reducing any perceived authority bias.
As a reviewer, you need to maintain a sense of honesty above all else in a review. If you do not understand something, ask either the author or another reviewer. The same rings true if you disagree with an approach the author has taken. This is okay to voice but as a reviewer, the burden falls onto you to be sure you disagree because of a core flaw with the approach and not because it's a personal preference to you.
Pro-tip: When in doubt, ask another reviewer for a sanity check.
In the situation where you find there are unknowns in the merge request you are reviewing, consider what the absolute worst-case scenario would be. If the very worst-case leaves this merge request with a net profit (and no permission-to-play issues), consider leaving a non-blocking comment to share your knowledge and collaborate.
At GitLab, we aim to be as inclusive as possible and this includes making space for people to grow and learn in their own way. As a reviewer, encourage authors to ask questions, make mistakes, and be fearless with their attempts to make something awesome. While we want the highest quality of code at GitLab, we also need to leave space for compassion, which creates the safety net authors deserve when they hit that submit button asking for a review.
If you find yourself working on a merge request that is not going to meet the quality benchmark, aim to let the author absorb that as a learning experience. But also try to help foster the right response of being inspired to try again with another, better merge request, not fear of being seen as lacking in some way.
As the author absorbs the learning experience, allowing yourself as the reviewer to also embrace this as a learning experience. Each problem encountered is an opportunity for the author and everyone on the team to learn.
Perfectionism is the enemy of progress. (Winston Churchill)
Nitpicks are requests for minor changes that address personal preferences or negligible code improvements.
Nitpicks are born from a wonderful place for most reviewers of wanting to ensure they provide the most end-to-end review possible and highlight every single potential issue. But they come at a cost:
Nitpicks can create a lot of noise in a code review. They can, at best, bog down a review, or at worst, create a toxic experience for the author.
If you, as a reviewer, notice that all you have pointed out are nitpicks, consider approving (and merging) without adding an additional review cycle. You can leave feedback but don't need to block the merge request from being merged.
When writing a feedback comment, please evaluate if the comment is a nitpick. While Paul Slaughter has a wonderful method for helping to inject context into your comments via conventionalcomments, it's always worth sanity checking if the nitpick really makes a positive net difference for the change.
To see more reasons on this topic, check out the stop nitpicking blog post.
