Practice: Pull requests

Self-check questions

Answer each in your own words first, then open the answer to check.

Q1. Who are the five audiences that read a PR description over its lifetime, and which one matters most for your motivation to write a good description?

Show answer

The five audiences: (1) the immediate reviewer, (2) future engineers debugging via git blame six months from now, (3) product managers tracking what shipped, (4) auditors verifying against tickets, (5) possibly an LLM summarizing release notes. The audience that matters most for your motivation: the future debugger. They will read your description without you available to clarify; the others can ask follow-up questions in the moment.

Q2. You’re about to merge a feature branch. The branch has 14 commits, half of which are WIP-style (“wip”, “fix tests”, “another fix”). Which merge strategy is the best choice and why? What would you choose differently if the 14 commits were all clean and meaningful?

Show answer

Squash. The 14 WIP commits would clutter main with noise that nobody needs to see; squash collapses them into one clean commit. If the 14 commits were all clean and meaningful, the choice depends on team convention: merge commit (preserves the work as a unit) or rebase (linear history, all commits preserved). The team default should drive this; if no default, lean merge commit because it preserves the explicit “this is one feature” framing.

Q3. A reviewer leaves the comment: “this function is bad.” Without writing code, identify three things wrong with that comment and rewrite it in a way that respects the author and is specific.

Show answer

Three things wrong with “this function is bad”: (1) it’s not specific (what’s bad?), (2) it judges the function as a whole rather than identifying a fixable issue, (3) it offers no path forward for the author. A specific rewrite: “This function does three things: parse input, validate, and persist. Splitting them would make each piece testable on its own. Optional: pull validate() out as a helper.” Now the comment is specific, focused on a structural concern, and offers a path forward.

Q4. You are the PR author. The reviewer requested a change. You disagree with the change. What do you do, in order? (At least three steps.)

Show answer

Steps:

Re-read your own reasoning. Were you right because you thought it through, or because you defaulted? If it was a default, the reviewer’s pushback is useful.
Re-read the reviewer’s comment. Did you understand their concern? If not, ask a clarifying question first.
If you still disagree, respond with explicit reasoning: “I considered X but went with Y because [specific reason].” Lean on principles, not preferences.
If after 2-3 rounds you and the reviewer haven’t converged, take it to synchronous discussion (call, hallway, Slack DM). Async disagreement loops sour relationships.
If you’re a junior dev and the reviewer is senior, weight their judgment more heavily but still articulate your reasoning; that’s how you grow.

Q5. Pick three of the 8 PR anti-patterns from the lesson and explain, in your own words, what they cost the team over time.

Show answer

Mega-PR cost: Reviewer can’t engage meaningfully, so they rubber-stamp (review becomes fake) or stall (the PR sits for weeks). Either way, the review’s purpose (catching issues, transferring knowledge, gating quality) is defeated. Multiply across the team; quality drops invisibly.
No tests cost: Reviewer has to either trust the author’s claim that the code works, or manually verify themselves. Trusting introduces bug risk; manually verifying wastes reviewer time. Either way, the PR’s purpose (gated quality) suffers. Multiply by every test-free PR; the team’s regression debt accumulates silently.
No description cost: Reviewer has to read every line plus git history plus Slack scrollback to reconstruct intent. Multiply by every PR; the team’s review throughput drops 30-50%. Future debuggers face the same cost on every line of the merged change.

PR description drill

This is the most pedagogically valuable drill in the lesson. Do it on a real PR if you can.

Drill 1, write a PR description from scratch:

Open a real PR you have at hand (or imagine a small change you just made). Write a PR description following the canonical structure:

What this changes
Section titled “What this changes”
Why this matters
Section titled “Why this matters”
How it was tested
Section titled “How it was tested”
Notes for the reviewer
Section titled “Notes for the reviewer”

Constraints:

Each section is at most 4 sentences. Cut ruthlessly.
The “What” section says what the code does, not what the diff includes. If you find yourself describing the diff line-by-line, you’re at the wrong level.
The “Why” section gives the motivation. If you can’t articulate why, the change might not be worth making.
The “How tested” section is concrete. “Tested manually” is not a description; “reproduced the bug with three test accounts, verified the fix across Chrome / Firefox / Safari” is.
The “Notes for the reviewer” section pre-empts the questions you expect them to ask.

Drill 2, review your own description:

Read your description as if you are the reviewer. Three questions:

Could you approve this PR based on the description alone (without reading the diff)? If yes, your description is too vague; reviewer should still need the diff.
Could you understand “why does this code exist?” in six months? If no, add context.
Is anything in the description false or aspirational (“will be tested by…” when it actually won’t be)? Fix or remove.

Drill 3, compare to a real PR you’ve seen recently:

Find a PR in your codebase (or any open-source project) and read its description. Score it on:

Does it have a clear “what / why / how tested” structure?
Could you understand the change without reading the diff?
Is anything missing that a future debugger would need?

Most PRs will score badly. That’s a feature, not a bug; recognizing badness in someone else’s PR is how you learn what to do better in your own.

Code review etiquette drill

Drill 4, rewrite bad review comments:

Each of these is a real comment from a real PR. Rewrite each to be specific, kind, and principled.

“This is wrong.”
“Why didn’t you use the existing helper?”
“lgtm”
“Can we just not do this?”
“Did you test this?”

(See cheatsheet for model rewrites.)

Drill 5, identify blocking vs non-blocking:

For each comment below, decide whether it should be blocking: or nit: (non-blocking):

“This variable name is misleading; consider renaming to userIds since it’s a list.”
“The function doesn’t handle the case where the input is null; will crash in production.”
“Prefer trailing commas for diff cleanliness.”
“The SQL query uses string concatenation; this is a SQL injection vector.”
“Consider extracting this into a helper for testability.”

Scenario reflections

Scenario A. You receive a PR review with 23 comments, half of which are style preferences (“prefer single quotes,” “missing trailing comma”). Your team uses a linter that should auto-fix most of these. Without writing code, describe the meta-conversation you should have with the reviewer (and possibly your team).

Show answer

The meta-conversation: “I appreciate the thorough review. Half of these comments look like things the linter could catch; would you be open to enabling auto-fix on the linter so we save review cycles for substantive concerns?” That’s the immediate response. The team-level conversation: “Our review cycles are spending significant time on style preferences. Could we add an auto-formatter (Prettier, gofmt, etc.) to pre-commit hooks so we don’t review style in PRs at all?” The team-level fix saves hours per week across all reviewers.

Scenario B. A teammate consistently opens PRs with no description, just a title. You’re tired of reading every line of their diffs to understand intent. Without confrontation, describe a path to changing this team norm.

Show answer

Path to changing the team norm: (1) Lead by example, your own PR descriptions are visibly thoughtful, so it’s clear what the norm could look like. (2) In one teammate’s PR, leave a friendly comment: “Could you add a brief ‘what / why’ summary? Easier for me to review with context.” Frame it as your need (helps you review faster), not their failure. (3) If multiple teammates have the pattern, raise it as a team retrospective topic: “we’d review faster if PRs had short descriptions.” (4) Consider a PR template (GitHub supports .github/PULL_REQUEST_TEMPLATE.md) that auto-prefills the canonical structure. Template makes the norm structural, not interpersonal.

Scenario C. Your team has two reviewers who consistently take 3+ business days to review PRs. Velocity is suffering. Without playing blame games, describe what you’d raise with your engineering manager and what data you’d bring.

Show answer

What to bring to the engineering manager: (a) data, “in the last 30 days our median PR review time was X days; team A’s average was Y, team B’s was Z.” Not “reviewer Q is slow.” (b) impact, “the lag is correlated with feature shipping delays; we’ve had N PRs sit more than 5 days.” (c) options, “could we set a SLA (e.g., review within 1 business day), rotate review duty, or hire/onboard a second reviewer for high-traffic areas?” Bring data plus impact plus options. Avoid blame; the goal is structural fix, not personnel feedback. Personnel feedback is what 1:1s are for.

Flashcards

Q. What is a pull request?

A proposal to merge one branch into another, surfaced as a piece of UI on GitHub / GitLab / Bitbucket. It’s also the place where code review happens, decisions get recorded, and the team agrees on what is shipping.

Q. What are the four sections of a canonical PR description?

## What this changes / ## Why this matters / ## How it was tested / ## Notes for the reviewer. Optional sections: Screenshots, Breaking changes, Migration plan, Rollback plan.

Q. What are the five audiences for a PR description?

(1) The immediate reviewer, (2) Future engineers debugging via git blame six months from now, (3) Product managers tracking what shipped, (4) Auditors verifying changes against tickets, (5) Possibly an LLM summarizing release notes.

Q. What are the three merge strategies and what's the basic shape of each?

Merge commit (creates a merge node with two parents, preserves branch history). Squash (combines all branch commits into one new commit on main, linear history). Rebase (replays branch commits as new commits on top of main, linear history with all commits preserved).

Q. Which merge strategy is the most common default for modern teams, and why?

Squash. Reasons: cleans up WIP commits automatically, produces linear history that’s easier to read, single-commit-per-PR mapping that’s auditable, works for short-lived feature branches.

Q. What's the difference between a nit: comment and a blocking: comment?

nit: is a non-blocking style preference; the author can accept or decline without holding up merge. blocking: is a must-be-addressed concern (correctness, safety, security). Tagging the difference saves authors from guessing.

Q. When should an author respond to review comments?

Every comment, even with “good catch, fixed in <commit-sha>.” Silence on a comment looks like avoidance. For comments you disagree with, explain your reasoning specifically.

Q. When should a reviewer approve a PR?

When the code is safe to merge, not when it’s perfect or exactly how the reviewer would have written it. Withholding approval for style preferences burns trust with authors.

Q. What are three of the 8 common PR anti-patterns?

(1) The mega-PR (too large to review meaningfully). (2) The unrelated changes PR (multiple logical changes bundled). (3) The PR with no description (reviewer has to guess intent).

Q. Why are PR descriptions institutional memory?

Because future debuggers find the PR via git blame and read the description to understand “why does this code exist?” A good description answers that question in minutes; a bad one costs hours per investigation, repeated for every future debugger of every line in the PR.