Writing a diff/PR description to get better reviews

During a recent work meetup, we had an impromptu exercise where we went over what makes a GitHub PR description most useful to the people reviewing it, particularly when not everyone is working on the same part of the codebase. Better descriptions make for faster reviews! This is my attempt to turn that discussion into a post.

More than just improving review speed, though, these tips should also make it easier for future developers (maybe you!) trying to understand what changed and more importantly, why. The “why” of a decision is so easily lost as links die and people forget.

The title

The title of a change should quickly convey what is affected. We have tons of code at Automattic and it’s easy to end up looking at a change with your mind somewhere completely different. The title can help to ground a reviewer so they understand what they’re looking at.

1. Prefix the title with what section of code or system is being affected. Examples are Shopping Cart:, Manual Payments:, Checkout:, etc. Sometimes this is not possible or not necessary if the rest of the title conveys the information clearly, but it’s a helpful shortcut when you want to describe something inside that system. (Some folks like to use additional markers like feat from Semantic Commit Messages and while these are nice but I personally have not found them as useful outside of commits. Convince me otherwise!)
2. Write the title in the imperative, start with a verb, and do not end with a period. This is the same advice commonly given for commit messages. The imperative tone reads more clearly when looking at a list of changes. Examples are, Add new field to checkout form, Reduce number of re-renders in contact details, Fix race condition while saving cart. Try not to use indicative or past-tense like Adds a new file or Fixed bug. No trailing period is needed because the title is always displayed separately from other content like the title of a book chapter or newspaper article.

Title example

Manual Payments: Replace 'change ownership' dropdown with input

The body

The body of the PR will vary considerably based on the nature of the changes but generally should answer three questions:

1. How did the system being touched work before this change? We only need to describe the necessary parts, but do give some basic context (like this) for someone who’s never seen that system before! (You and your team may know the system inside and out right now but after a few years pass, you’ll have no idea what any of this means.)
2. What is the problem with the system just described that this change is intended to resolve or improve? This is often easy but can sometimes be complex to explain well (like this). Do not rely only on a link to an issue or a Slack thread.
3. What does this change do and how does it resolve the problem just described? A set of “before” and “after” screenshots can often be worth a thousand words in some circumstances (like this).

These three things don’t always have to be in that order, and they don’t need to be very long, as long as the context and reason are clearly conveyed (like this). It’s far too easy to just write the third part and bypass the other two.

For additional context, provide links to related P2 posts, issues, or even Slack threads, but don’t rely on those links alone. They should be like footnotes in a book; useful if someone wants to learn more but not necessary to understand the meaning. Maybe even directly quote those sources if something on the other side of a link is particularly useful. You never know when a link will stop working or who might not have access to it.

And don’t worry about writing too much! As we say at Automattic,

It’s perfectly OK to spend more time crafting your commit message than writing the code for your commit.

It’s perfectly OK for your commit message to be longer than your commit.

Another way to explain all of this is,

In most cases, you can leave out details about how a change has been made. Code is generally self-explanatory in this regard (and if the code is so complex that it needs to be explained in prose, that’s what source comments are for). Just focus on making clear the reasons why you made the change in the first place—the way things worked before the change (and what was wrong with that), the way they work now, and why you decided to solve it the way you did.

https://cbea.ms/git-commit/#why-not-how

Body example

The "Change ownership" modal in the Manual Payments form includes a dropdown menu of all users who have created a manual payment. However, to do that, it calls `get_list_of_payment_creators()` which loops through all such users and calls `get_userdata()` on each. This process can cause PHP to run out of memory before it completes, throwing a fatal error.

In this diff, we change the modal to use an input field instead of a dropdown. This is less convenient but will keep the page from crashing while we determine a better solution. For convenience, the input field is pre-filled with the current user's ID like the dropdown menu before it.

[Fixes the issue reported here]

[Before screenshot]

[After screenshot]

The testing instructions

There are many different kinds of changes, and they require different sorts of testing and review. Sometimes a change cannot really be tested manually and we must rely on automated tests. Sometimes we cannot even do that and must rely on pure code review. However, if you want to get a useful review from people who aren’t intimately familiar with the code, it’s a good idea to give them as much help as possible.

1. Make as few assumptions as possible about what your reader knows how to do and list what assumptions you can (eg: having a Jetpack site, having a new user account, having an email product subscription). This can be surprisingly hard! You probably know the code so well that writing step-by-step instructions seems like a waste of time (it’s not).
2. Provide links, command-line instructions, or inputs that can be copied and pasted for the reader. If these include data that needs to be customized (eg: blog ID, user ID, etc.), make sure that this is clear and, if you can, provide functional examples. For instructions that involve UI, provide screenshots. For testing that requires modifying the code itself (eg: to force a rare condition), provide a context diff (like this).
3. Keep each instruction as simple as possible and split complex instructions into multiple steps. Use numbered steps if there are a lot so the reviewer can more easily keep their place. Split up multiple test cases into their own sections with their own bullets or step numbers (like this).

One way to make all of this relatively easy is to write down/screenshot/copy-paste each thing you did when you were testing the change yourself, but remember to proofread it in the shoes of someone who doesn’t know what you’re talking about.

I often find that while doing this I discover test cases I hadn’t considered or ways I could make the change more clear, so in the end there’s benefits to more than just the reviewer.

Testing instructions example

1. Create a manual payment by following the steps [here].
2. Verify that the created manual payment page displays without errors (previously this would cause a fatal).
3. Click to change the ownership of the payment method. Enter the userid of a different owner and save the form. Verify that the change works correctly.

The challenge

Most developers, myself included, spend a lot of energy preparing and writing code. When it comes time to post that code for review, we often don’t have a lot of energy left. Not to mention that the act of writing prose is quite different from coding and may require a significant context switch to do well.

While not always possible (or needed), let’s try to have compassion for our readers and treat the writing of change descriptions as seriously as we do the code itself. This may mean taking a break before submitting it for review. It may help to pretend that you are explaining your change to a non-developer friend and giving them instructions for testing it. If they can do it, then your actual developer friends should do great!

Do you find something not mentioned here useful in diff or PR descriptions? Please let me know in the comments!