Code review: roles and etiquette
Intermediate
Why this matters
Review is where most of a team's communication happens, and it is the activity where tone has the largest effect on outcome. The mechanics are in lesson 9.8 and lesson 10.8; this lesson is about the human half, which is the half that decides whether review is useful or exhausting.
Who is responsible for what
| Role | Responsible for |
|---|---|
| Author | A change that is small, explained, self-checked, and answered when questioned |
| Reviewer | Reading it properly, saying what they checked, and being timely |
| Maintainer | The final decision, the conventions, and protecting the branch |
| QA / tester | Whether it actually works, and whether anything else broke |
| Technical writer | Whether it is documented, understandable, and consistent with the rest |
Most review friction comes from someone doing another role's job: a reviewer redesigning the change, an author defending rather than answering, a maintainer merging around a review. Naming the roles makes it easier to say "that is a maintainer decision" without it being personal.
The author's job
Make it reviewable. A change of thirty lines gets a real review; a change of three thousand gets a rubber stamp or a three-day delay. If it is large, split it.
Explain why. The diff shows what changed. The description says why it was worth changing, and how the reviewer can check it (lesson 9.7).
Review your own first. Read the diff before requesting anyone. Half the comments you would have received are things you will spot yourself: a debug line, a stray file, a renamed thing you missed in one place.
Answer everything. Every thread gets a reply, even "done" or "kept as is, because…". Silence reads as ignoring, and it is the most common cause of a reviewer becoming reluctant.
Do not take it personally, and do not write as though the reviewer might. Comments are about the change.
The reviewer's job
Be timely, or say when you can. A merge request waiting two days is a person blocked for two days. "I can look at this tomorrow morning" is a complete and useful response.
Say what you looked at. "I reviewed the documentation; the Python needs someone else" is a better review than a silent approval, because it tells the maintainer what is still uncovered.
Separate must from prefer. Prefix opinions: "nit:", "non-blocking:", "optional:". Without it, every comment reads as a requirement, and authors either over-comply or start ignoring the lot.
Suggest, do not describe. For wording, write a suggestion the author applies with one click rather than a sentence about what is wrong with the current text.
Ask rather than assert when you might be wrong: "What made you choose X? I ask because Y" invites an answer; "This should be X" invites a defence.
Say one good thing. One line, honestly meant. It changes how the rest of the review reads and costs five seconds.
Disagreeing
Disagreements in review are normal and usually productive. The pattern that resolves them fastest:
- State the concern once, with the reason and the consequence: "this will break saved scripts because the option is renamed".
- Let the author respond. They may have information you do not.
- If you still disagree, name the decision-maker. "I think this needs a maintainer's call, adding Dev."
- Accept the outcome and say so in the thread, so the record shows it was decided rather than dropped.
What does not work: repeating the concern in three threads, escalating in chat where there is no record, or approving while privately disagreeing.
Reviewing as a non-developer
You are qualified for more than you may think, and unqualified for some of it, and saying which is which is the professional move.
| You can review with authority | What to say |
|---|---|
| Wording, clarity, tone, grammar | Comment or suggest directly |
| Whether the docs match the change | "The code defaults to 10 now but the guide still says 5" |
| Whether an error message helps a user | "A user seeing this will not know what 'nil ref' means" |
| Whether the change is documented at all | "Should this be in the changelog?" |
| Whether the steps work, by following them | "I followed these on Windows and step 3 failed" |
| Whether it matches what the issue asked for | "Issue #12 also mentioned the FAQ link" |
That last row is worth noticing: checking the change against the issue is a review nobody else reliably does, and it catches real misunderstandings.
For the rest, say so plainly: "I reviewed the documentation only." That is a useful review, and it prevents your approval being read as covering more than you checked.
How to do it
The most valuable thing a non-developer reviewer can do is check out the branch and follow the instructions:
$ git fetch
$ git switch docs/12-windows-install-stepsThen actually run the steps. Reviewing instructions by reading them finds typos; reviewing them by following them finds the missing step.
The platform extension shows the diff and threads in the editor, with the surrounding file visible, which is the main advantage over the web view for judging whether a change is consistent.
The Merge Requests or Pull Requests tool window does the same, and its diff viewer is the best of the three for large changes.
Batch comments with Start a review, use Insert suggestion for wording, and Approve only for what you actually checked. Whether unresolved threads block the merge is a project setting (lesson 9.5).
The same, plus a blocking Request changes verdict. Use it for factual errors and broken steps; use Comment with "non-blocking" for preferences (lesson 10.8).
Common mistakes
- Approving without saying what you checked, which implies more than you did.
- A dozen separate comments instead of one batched review.
- Unmarked preferences, which read as requirements.
- Redesigning the change in review. If it needs a different approach, say so once and let the author decide how.
- Silence on threads, as the author.
- Escalating in chat instead of in the merge request, leaving no record.
- Reviewing instructions by reading them. Follow them.
Try it yourself
Goal: give a review that is scoped, batched and useful.
- Find a merge or pull request on your team's project, or make one on your practice project with a deliberate error.
- Check the change against the issue it claims to close. Note any gap.
- Check out the branch and follow any instructions it changes.
- Leave one batched review: at least one suggestion, one question phrased as a question, and one sentence saying what you did and did not review.
- Note how long it took. For a small change it should be under fifteen minutes.
Expected result: a review that says what was checked, proposes exact wording where wording is the issue, and leaves the author with a clear list.
Show solution
Step 2 is the one to keep. Comparing the change with the issue is a check almost nobody does, it needs no knowledge of the code, and it catches the most expensive class of mistake: work that is well made and solves the wrong problem.