Pull Request Descriptions

Written by coxley on

Great descriptions are the first line of defense for healthy code review culture. PRs are reviewed faster and more easily, future changes can be made with confidence, and they set the tone for other engineers on the team. Online discourse may say summaries are misguided when changes are linked to tickets and teams are aligned, but my experience has not reflected that.

#

Intro

Great descriptions are the first line of defense for healthy code review culture.

PRs are reviewed faster and more easily, future changes can be made with confidence, and they set the tone for other engineers on the team. Online discourse may say summaries are misguided when changes are linked to tickets and teams are aligned, but my experiences have not reflected that.

#

Questions to Answer

So what makes one good, and how can we reproduce it? There are a few top-level questions each PR should answer.

Why was this change needed? This should be more than linking to a ticket. Be explicit and tell how your change connects to solving a problem. This context helps reviewers understand the problem space, and even suggest better alternatives. Without knowing the root issue, reviewers are just evaluating code by its own merit — which we do not want. Code can be correct in isolation; yet wrong in the environment.

Explain your approach, showing how your solution solves the problem above. I find performing a self-review of your changes useful. By reading the diff — top to bottom — you can catch bugs, unneeded TODOs, commented code, and themes you’d have otherwise forgotten to point out.

Point out alternatives you considered and why you decided against them. This let’s reviewers understand your thought process, while simultaneously answering questions they may have asked. Arguing against yourself solidifies your understanding of the problem and may even lead to better solutions. Since everything is a trade-off, showing the ones you made helps everyone — present and future.

Are there any known caveats with your approach? Tell us about them!

Demonstrate a reproducible way of testing the change. If you vetted the change by running some local commands, include the exact commands and their relevant output. If it’s more visual, include a screenshot or a video. Hell, include both.

Tests being reproducible is key. Not only does it force you to understand what other things your change affects, it teaches future commit authors how to test similar changes. Being able to copy-paste a test plan from a previous PR saves a ton of time when not everyone regularly touches that project.

Showing in detail how you tested will also give reviewers an opportunity to see blind spots and suggest improvements.

The ticket still has a place – but it is not a substitute for a well-thought summary. Tickets are the context of why something had to be done. A pull request description is everything about the change and how it relates to what’s around it.

Answering all of these questions up-front makes faster, more accurate reviews. There are simply less round-trips needed. Nobody likes to wait hours for a review to get a few questions asked, answer them, and wait hours more.

We limit round-trips to other services in software, and I argue we need the same for code review.

#

Requesting Changes

You should feel comfortable asking for a better description — it’s a part of the pull request.

How this is received varies from culture to culture, but that should be squashed. Requesting changes in a PR isn't a personal affront. It's basically the point. If a description looks like these, there are opportunities for improvement!

Copied!
# Summary

Adjusted filter for alert

# Test Plan

CI/CD

---

# Summary

Improve SQL performance

# Test Plan

tests

Neither of the first examples tell us why the work was needed.

Sure, the alert conditions were adjusted but six months later we can't remember why. So no one changes it out of fear of losing a critical monitoring piece. Or if you write raw SQL a lot, the reason performance is improved is obvious. But maybe not for your junior engineer or someone that's always relied on ORMs.

People that were in standup that day or week will know, but what about next month? Next year? Folks leave, context is lost, and it’ll be hard to know whether something can be reworked without the logic behind decisions.

#

Templates

I despise complex PR templates.

While they come from an honest place to enforce consistency, what actually happens is people leaving the templated text/checkboxes as-is. This results in a description with a lot of words, but few of them matter. Engineers should describe their problem and solution in their own words.

My preferred template is basic:

Copied!
# Summary

# Test Plan