About the author
Gonzalo Bañuelos has been developing software for over 25 years at companies such as Procore Technologies, Symantec, BigCommerce, and Cars.com. He’s a Stanford CS graduate and has been a PullRequest reviewer since July 2019.
Follow Gonzalo on Twitter @gbanuel and read more of his work on Medium
The better we present our code for review, the more likely the reviewer will be engaged with a critical eye. One of the ways we present our code for review is through the Pull Request (or Merge Request if you’re a GitLab user).
The Pull Request (PR for short) is the method by which — specifically using Git and GitHub — you can loop interested parties into reviewing and then approving your change, then merging it into some branch (presumably the trunk). This is where you explain the “whats” and “whys” of your code.
For Our Purposes
I’ll be using as an example for the remainder of this post the case where you’ve added code to support users and user authentication. I’ll also be assuming it was written for a Ruby on Rails app.
Your PR Description Matters
Use your PR description as an opportunity to set the reviewer up for a review they will want to do, a review they will know how to review. Here you’ll get to explain what you’ve done, why you’ve done it, and how to prove it is ready to be merged into the main trunk. So let’s break this down a bit…
The Template
Whether you’re reviewing code for a project you work on every day or a code base you’re completely new to, having a PR template is always helpful. And as a programmer, it’s a good reminder of what the developers reviewing your code will need to know to best review it.
It’s fairly easy to set up a template that you and your teammates can use to formalize the PR process. Version control providers like GitHub support automatic template injection in the description of every pull request created. See PR Templates.
A favorite, simple template of mine:
## What?
## Why?
## How?
## Testing?
## Screenshots (optional)
## Anything Else?
As you can tell, it’s not rocket science. It’s not even computer science. It’s just a clean, easy to understand synopsis of your work.
The What
Explain the changes you’ve made. It doesn’t need to be fancy and you don’t have to get to technical, yet. Just explicit prose on your net change will typically suffice. At a high level, this is where you let the reviewer know the overall effect of the PR. Reference a ticket in your issue tracker if appropriate, but by all means, don’t just reference the ticket. It’s important to explain what the change is and then and only then reference the ticket. It’s a much better experience for the reviewer if they’re able to spend more time reviewing code and less time studying specification that may not even be applicable on the code level.
Again, be explicit.
Here’s a few examples of bad “what” explanations:
## What?
Support for #JIRA-123
## What?
Added table to support #JIRA-567
## What?
See the subject.
This is a particularly annoying one that I see from time to time.
A bit better of an explanation:
## What?
Added support for authentication. #JIRA-123.
An even better explanation:
## What?
I've added support for authentication to implement Key Result 2 of OKR1. It includes
model, table, controller and test. For more background, see ticket #JIRA-123.
Again, try to be explicit and try to capture the changes in a few short, concise sentences that don’t require more than a few seconds to grasp.
The Why
The “why” is sometimes more important than the “what” of a code change, but we tend to put it after the “what” since we aren’t evaluating theory, we’re evaluating tangible code changes.
The “why” tells us what business or engineering goal this change achieves. It’s the reason we get paid as developers. The “why” is a chance to explain both the engineering goal, but also a some business objective that is satisfied or moved along.
Imagine a situation where you are adding a simple environment variable default value. Less than a line of code. Pretty straightforward and short, right? It’s almost self-explanatory. Except it completely changes the nature of the libraries used in your app. Then it’s probably worth a comment.
These are poor examples of “why” explanations:
## Why?
See #JIRA-123
## Why?
See 'what' section.
## Why?
Bob the PM knows about this.
Nothing here helps the reviewer see the bigger picture (and the last example simply kicks the can down the road).
Better:
## Why?
These changes complete the user login and account creation experience. See #JIRA-123 for more information.
Don’t be afraid to use complete sentences and an active voice. This is living, running code in the present, not the past.
The How
Of course, the PR diff will tell most of the story of the “how”, but make sure to draw attention to the significant design decisions. You decided to write a recursive method instead of a loop, pointing out the merits of this will help the reviewer understand your reasoning and in turn provide a better review.
Bad:
## How?
Wrote migration and model.
Well, that’s pretty obvious by looking at the code. Help the reviewer out with a little more info.
A better version:
## How?
This includes a migration, model and controller for user authentication. I'm using Devise to do the heavy lifting. I ran Devise migrations and those are included here.
Testing
If your team includes applicable tests alongside updates to your code (and you should), merging code without tests or changes that cause existing tests to fail will typically get caught by a teammate or build management tool. You may optionally move the tests to another commit and another PR, but that fans out your liability if things go wrong. Also, you won’t be taking advantage of your CI/CD process if you merge in separate batches, as the first, test-less commit won’t even cause your pipeline do anything different. Worse yet, that first batch may make it to production before a single test is ever run on it in your pipeline.
Some code, especially infrastructure code (say HELM or Kubernetes yaml files) are harder to test. So it’s important to let the reviewer know how you tested them in case you can’t check in tests. Alternatively, you can explain to the reviewer how to test it locally if necessary. Showing the results of tests you’ve run in this section if none are visible in the diff is also very helpful.
Let the reviewer also know if some conditions or edge cases were not tested, why they weren’t tested, and how likely they to occur, and if so, any associated risks.
Screenshots
Of course, screenshots are especially helpful for UI-related changes. A simple screenshot of the before and after, or of the current state vs. your local development view, helps the reviewer tremendously. They’re some of my favorite reviews to do.
Even backend code can benefit from a screenshot of the net change. Take for example the result from a CLI tool after you run the code you wrote. Imagine a snapshot of memory layout. The numbers won’t match up when that same code is run on someone else’s box or in a different container, but they still mean something when viewed as a whole. This is a great spot to put a snapshot of that result and help your reviewing teammates out.
For infrastructure code, you may want to either snapshot or copy the result of say a Terraform plan here. If you want to avoid polluting the comment with a huge plan output, use a collapsible section if your version control provider supports it. Remember, this is markdown and you can get creative. Here’s how to do it in GitHub:
<details>
<summary>Terraform Plan</summary>
## After running plan:
</details>
Anything Else?
You may want to delve into possible architecture changes or technical debt here. Call out challenges, optimizations, etc.
In our running example, let’s say you know of a better way to offload this work altogether. Use this opportunity to call attention to it:
## Anything Else?
Let's consider using a 3rd party authentication provider for this, to offload MFA and other considerations as they arise and the privacy landscape evolves. AWS Cognito is a good option, so is Firebase. I'm happy to start researching this path.
Let's also consider breaking this out into its own service. We can then re-use it or share the accounts with other apps in the future.
Putting A Bow On It
Let’s take a look at our PR description now…
## What?
I've added support for authentication to implement Key Result 2 of OKR1. It includes model, table, controller and test. For more background, see ticket
#JIRA-123.
## Why?
These changes complete the user login and account creation experience. See #JIRA-123 for more information.
## How?
This includes a migration, model and controller for user authentication. I'm using Devise to do the heavy lifting. I ran Devise migrations and those are included here.
## Testing?
I've added coverage for testing all new methods. I used Faker for a few random user emails and names.
## Screenshots (optional)
0
## Anything Else?
Let's consider using a 3rd party authentication provider for this, to offload MFA and other considerations as they arise and as the privacy landscape evolves. AWS Cognito is a good option, so is Firebase. I'm happy to start researching this path. Let's also consider breaking this out into its own service. We can then re-use it or share the accounts with other apps in the future.
This is a fleshed out comment that gives the reviewer good context on how to proceed to review the code. As a reviewer, I know from this description that I need to look at the new model, controller and table. I also know that security is a concern since we are using Devise, and as such I’ll have my antennae up and sniffing for vulnerabilities. Lastly, I know to look at the test coverage and challenge whether or not all of the methods are being tested.
As a bonus, I know that this code also presents options that I can then go and research to later bring to the attention of my leads.
Even if you include the optional bits, you still have a pretty concise description that doesn’t take long to read and communicates a lot of useful information. It also reads conversationally or like prose, not a series of terse, cryptic and acronym-filled fragments.
## What?
I've added support for authentication to implement Key Result 2 of OKR1. It includes model, table, controller and test. For more background, see ticket #JIRA-123.
## Why?
These changes complete the user login and account creation experience. See #JIRA-123 for more information.
## How?
This includes a migration, model and controller for user authentication. I'm using Devise to do the heavy lifting. I ran Devise migrations and those are included here.
## Testing?
I've added coverage for testing all new methods. I used Faker for a few random user emails and names.
Going Too Far
For those of us who lean a bit on the wordy side, be cautious of being unnecessarily verbose. I’ve seen some PR descriptions that go well beyond what is necessary to get the code change framed. They’ll include paths of executions, truth tables, you name it. This is too much to read, and it often indicates a check in that is too complex to understand and should be broken down a few times. Small, agile increments of code should be about the size of your PRs.
Nothing strikes more fear (or anger) into a reviewer than having to read 2000 lines of brand new code, and letting that PR merge only to feel like they barely had time to gloss over most of it because of other demands on their time.
We’re Only Human
Keep it simple. You are communicating with a person, not writing code all over again. And writing a long, complex description is just creating more work for the person on the other side. Short, concise and descriptive PRs get reviewer juices flowing, and ultimately move the process more smoothly along.
If you found this useful, we encourage you to check out Gonzalo’s piece Law of Demeter and Ruby’s “&.” operator.
