Rob Watts

date: 2021-04-01T20:45:15.133Z title: How to write PR descriptions summary: Wonderbly’s guide to the art of creating a good PR

The words below are part of the Wonderbly developer guide. I contributed a little but this is mainly the work of Hraban Luyat and Lev Perlman. They are great devs and I am grateful to have worked with them both.

Pull Requests are about telling a story. What kind of story are you telling? Is it the creation of a new feature, or how a bug has been fixed, or why an improvement needs to be made? Put yourself in the mindset of the reviewer, what does that person need to know to understand your story and be able to do their job.

Remember to balance this with the story told by the source code as a whole. Always judge what the source code will look like after merging your PR, and don’t lose track of its priority over this individual patch.

To help you tell that story, use these these guidelines as much as possible.

Writing a description

First step is creating a useful description. A good description helps your reviewer understand what they are being asked to look at.

The title that says concisely what this PR is

fix: no feedback after editing customisations

Remember: the title has a maximum length of 50 characters. This helps understand the purpose of the PR at a glance when looking at a long list of changes in history view, a typical way of looking at history to see what’s changed:

a10dd93 fix: statement timeout to 300s (#300)
714681a enable heroku nodejs analytics (#299)
12ee117 fix: orderEnricher: removing zip and merging sources correctly (#296)
bebfcf8 feat: stock - upserting stock with permissions (#288)
aa0b34e fix: async nock post request body test in CDS (#293)
ee2df10 feat: Stock Log Levels - change to info (#289)
f6eccc8 feat: psps deps: Passing AppDependencies to all PSPs (#283)
de54de2 docs: improve readme (#286)
8b0d6ec refactor: native request types, no need to redeclare (#284)
8f370fa feat: ecommOrigin in FullPspOrderData (#282)
7272d23 fix: OneFlow: shipping update path (#279)
b791d61 [Stock 403] - handling 403 error from OneFlow stock manager (#278)
5d21f49 Add printForReal boolean to order event log (#269)
050420f feature/ order log formatter (#277)
9e980b4 Fix sample unit test file (#276)

As you can see, some of these descriptions work better than others. Some don’t follow the convention and they immediately stand out; this is why it’s important to follow the convention in all submissions.

Have a good look at this list and evaluate which submissions tell you what you need to know (what changed, which files were touched, …), and which don’t. For those that don’t, imagine having to dig deeper into them and having to manually inspect each one to get a feeling for what actually changed.

As always, with everything you do relating to code: you are telling a story, so keep your audience in mind. What is their perspective, what do they want, who are they? In the case of PR titles, their perspective is this list of summaries. Keep this in mind when writing the title for your own PR.

The description that explains why this PR must exist

Explain why, with a note of what has changed.

When submitting edited customisations for a product after purchasing the user wouldn’t be navigated back to their order. This was broken during the changes made to customisations when trying to fix the case mismatch between Eagle, Website, and Muse. After looking at where the fetch method updateOrderItem was being used I noticed that the actual order response wasn’t being used so I have removed the .then following the call to Eagle.

Again: you are telling a story; who is your audience? It is people reviewing your PR, and people using git blame to see what happened to a specific line, then wishing to see it in the context of its entire patch. What would those different audiences need to know to fully understand the patch? This can include:

• Time pressure (“Should be doing X, but Rob says he wants it done by the end of the day so I must go with Y”).
• Alternative solutions that were not chosen (“Not doing Z because it might clash with Foo”). This is often overlooked but incredibly valuable.
• Links to related issues.
• Links to logs, or copies of logs
• Excerpts from e-mail threads with suppliers / clients
• on and on.

In the end, this is about “what is the context of this patch?” Most of that context could also be put in the code itself, which is more desirable if appropriate (remember the priority: code > patch). It is a delicate balance between what should live in the patch description, and what as a code comment. A rule of thumb is that comments should always go in the code, unless they are only relevant historically, but don’t have any potential to benefit future decisions. If a library was removed because it is unmaintained, it won’t risk being reintroduced, and there is little value commenting on it in the code. However, time pressure from management is almost always more useful as a code comment, because it helps future maintainers understand they can safely rewrite something, that there was no greater purpose to this hack than “getting it out the door.” It is an instance of Chesterton’s fence.

Think about whom this PR will affect

Consider who is impacted by this. Our users? If so, write down how. Is there something our customer support team need to know? How-about dependent systems, is there new behaviour to be aware of? Go through this exercise and you may find new dependencies you haven’t thought of.

Highlight danger

Highlight danger clearly.

• Is there a complication with roll-out beyond a simply deploy? E.g. a new environment variable, a dependent PR etc
• Can this be rolled back quickly? If not, make it clear
• Does this create potentially breaking changes? E.g. a breaking API update

Provide useful links

Link back to the original ticket. Share links so the reviewer can preview.

Overall, don’t make the reviewer work - any link that provides context can be helpful.

Acceptance criteria to show new behaviour

Help someone understand what has changed by adding acceptance criteria. Create a list, ordered in a way that makes sense for the reviewer.

Example:

• Editing a product from /personalized-products/:productSku/preview/:orderId/:itemId (edit in account) will redirect users back to their order
• The user will see their update has been successful on the order page

Include Steps to Test to walk the reviewer through the change

Show the reviewer some steps that demonstrate how acceptance criteria have been met.

For example:

1. Add a product to cart
2. Go through checkout (v1 or v2 doesn’t matter)
3. Once on the order page, click “login to edit order”
4. Click on the edit link in one of the line items
5. You should now be on the preview page
6. Make any change to the product and click save
7. You should now be navigated back to the order page and be able to see the new customisations

Don’t conflate concerns

Reviewers don’t want see changes that aren’t in the description. Adding “oh and I also refactored/cleaned-up a few classes” to the description doesn’t make it ok either.

Imagine being in the head of the reviewer, a PR that does many things (a conflation of concerns) is more change for the reviewer, therefore more things they might miss, therefore more things that may go wrong in production and therefore more time unpicking the changes when it does.

If your code does require a refactor - think whether you can isolate in a separate PR, and start with that. This way, the reviewer might receive several PRs with a smaller amount refactoring prior to receiving the main PR that includes the logical change or the new feature.

Consequently that main PR will be smaller, more isolated, more readable.

Use git to tell a story

This is a hard thing to do but it can make reviewing much easier and when the commits are squashed can provide a decent summary of what happened that we can all see our IDEs.

Writing a good commit message

Borrow this syntax from the Coventional Commits spec: <type>[optional scope]: <description>

Where type is one of fix, feat, or improvement.

Example:

fix: Eagle API: Remove handling of response from edit in account api endpoint

Putting commits together to tell a story

An ideal git log tells a linear story, each commit showing progress to completing the feature. It’s easy to show that progress when you have thought about what you plan to do before you start coding. This will help you, provide clarity to the PR and therefore help the reviewer. Then you can plan the commits as a series of improvements to the PR’s stated goal.

When it comes to review, the reviewer can step through every commit, rather than having to parse every file change all at once.

Provide some tests

Your PR should contain a test that validates the new behaviour. Think from the PR reviewers perspective: tests are documentation, they show new behaviour, what should happen what, importantly, what should not happen.

When writing tests, it’s not enough to prove your code works. Think “how could this go wrong?” and write tests to cover those cases as well. Think how someone could misunderstand or misuse your changes, make sure they are covered in tests as-well.

Add why not what comments to the code

Look at your code and if the reason why is not clear then add a comment, probably with a TODO to remind ourselves that something needs to be done.

If you’re adding comments to explain what this code is doing then stop and think again. This is usually a sign that your code is overly complex. We should be able to determine what the code is doing just by reading it.

Readable code that doesn’t work is better than unreadable code that does work!

The words below are part of the Wonderbly developer guide. All credit should go to Hraban Luyat. He’s an incredibly smart guy and you’ll be lucky to have him on your team.

This note is about the importance of writing code with other people in mind. Knowing that the poor sap who has to deal with your crappy code is a colleague you respect, someone you may never meet or (very likely) you.

Considering other people

What really, at its core, is programming about? What are the most common causes of friction and losses of efficiency in the programming process?

To gain some insight into this matter, make sure always to consider your coworkers. You write and contribute code not for yourself, but for your colleagues (and for your future self, who will have forgotten everything that present self knows). This includes colleagues who have not joined yet, colleagues who will join after you are gone, even after you and everyone presently working with you are gone. When all that remains is the work; how is it used, and how can it be made to optimally speak for itself?

All our dogma, every rule we have, all of it attempts to optimise for that scenario. That is where, we believe, the most fundamental gains in efficiency (and morale) can be achieved. This point of view underpins our entire methodology, and should be kept in mind when reading this document.

Great code is easily understood

The most important job that code has is: to be understood by your colleagues, both present and future.

Fulfilling its intended purpose correctly (aka “working without bugs”) is number 2; because understood, broken code can be fixed; but working, misunderstood code will break and then cannot be fixed.

In order to understand code, a programmer (your colleague, or future you) will navigate the code using those two perspectives. They tend to go through these phases, in order:

1. Read the least amount of actual code to get a vague idea of how it works, change the minimal amount of code to achieve their task, ignore everything else. Don’t read comments, don’t read docs, don’t read unrelated code.
2. If this doesn’t work: attempt to read some comments close to the code they think is relevant, maybe some related function definitions and types, some calling code.
3. Nightmare: the task still can’t be completed. Curse whoever wrote this spaghetti, a pox on who hired this tool. I am relegated to reading documentation, ugh. This troglodyte was apparently not aware of the maxim “good code is self documenting”.
4. I need to see who this idiot was. I have to see their face. Who could be this obtuse. What absolute waste of oxygen produced this nightmare. git blame. Oh, it was me 6 months ago.

Point being: people only open a project when they have a specific job to do. That task is their only priority, and they will (want to) spend the absolute minimum amount of mental energy learning about any idiosyncrasies of this project. Nobody will open this project just because. Nobody will know, nor care, about the conventions, about the unwritten rules, or even the rules written in bold letters in the README. Don’t get frustrated by this: it is wasted energy. Rather, accept it as true, and write code that is resilient to it.

Great code is easily understood. Above all else. It can be understood using those two dimensions: the code & comments themselves (1), and sometimes, in the extreme, the history of how it came to be (2).

Be demanding now to avoid burdening the future

As developers, we need to think of the code we write as our personal product with our fellow developers as customers. The reverse is also true, we must act as demanding customers when presented with our fellow developers code.

New code must be maintained, likely by developers who the submitter or reviewer will never meet. For that reason we must be mindful of burdening future developers with changes we make now.20

This note outlines a simple process for managing a backlog of work. It is basically a slimmed down version of Scrum. Or a mildly pimped up version of Kanban.

Who this is for

This process is best for small, multi-disciplinary teams who need a better way to manage their work. They probably have none to little existing process, like simple lists or shared documents. External folks are usually chucking in requests that are mounting up in a big, unprioritised list.

Bringing order to these teams is important for all the member’s sanity while bringing more predictability to everyone who depends on them.

The team’s unit of work: one week sprints

Sprints should last a week. It keeps momentum and people focused.

Two week sprints are great if you’re a mature team with good principles going deep on a multi-month projects. But it doesn’t work for small teams who want a simple process.

Everything is broken down into singular weeks. How did we do this week? What are we doing next week? Simple.

There’s no need for pointing or any of that agile stuff. It’s only a week, and team doing the planning is small. This means there is an instinctive sense of what can be achieved. When the work is broken down well, people can look at their work, think ahead and appreciate the size of it before committing.

The three boards

Managing backlog, up-next and in-progress items on one board is too much to manage.

Perhaps more can be achieved by a fancy tool but it’s usually better to use simple, accessible tools like Trello.

The roadmap board

The roadmap board looks at the next sprints. This is where we break down epics into work delivered week by week.

The roadmap board usually consists of these kinds of lists:

• Epics one list with cards for each major epic the team are working. An epic is anything that takes multiple weeks to deliver, usually this is the stuff that people outside your team will have actually heard of. Link these cards to the high-level requirements. Can also create a label for an epic and assign it to the cards you create.
• Backlogs work assigned to epics but not assign to a week yet. Also useful to create backlogs for Bugs or various house keeping tasks. Clean often otherwise they get too big and useless.
• Future weeks these are are lists for specific weeks, such as “Feb 1-5”. Have at least 3 weeks in the future planned initially. Get better over time and be able to specify further out as you gain predictability.

Having this split out from the main active board is usually the thing that makes a team feel under control. Backlogs are not growing endlessly and the amount of in progress work is not overwhelming.

The active board

This board is the more familiar one you would expect, except it only contains now and next stuff. This turns it from a mess into something can see and then take care of.

Everything in this board should represent a weeks worth of work, that unit of work everyone understands.

The lists here are as you expect:

• Todo / ready to start the tasks you’ve agreed to work on this week and no more. If you’re not working on it this week then take it off the board.
• In Progress the tasks that are in progress, moved by the owners of the cards when they start them so others know they are being worked on
• Blocked tasks that the owner cannot make progress on. This is bad. Deal with these ASAP and never let them back up.
• Done tasks assigned in this week and completed this week

You can add other lists like “QA” or “testing” or “PR review” in this list as-well, if that works with your process. Keep these lists to a minimum. Just focus on easy flow from left to right and highlighting blockages.

The done board

This is the best board - every week move the “Done” list from your active board here.

Every now and then take a peek at this board and see how much you’ve done.

The PM, aka the card whisperer

To really make this work you’ll need someone who has the responsibility to keep the process moving. Resolving blockers, filling up future weeks and assigning owners.

Ideally, this will be someone with Product/Project Manager in their title. However it doesn’t always work out that way, especially in small teams.

Whomever it is, the person should be senior enough to know enough detail about each card to make priority decisions. This person should focus on flow, keeping the team delivering according to the whatever priority is at time. This person can then relay progress back to the wider business.