Organizing Your Git Pull Requests
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.
The Pull Request 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 Comment Matters
Use your PR comment 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
I like to see a template used in the comment when reviewing a PR. As a programmer, it’s a good reminder of what you need to put into the comment in case you happen to forget.
It’s fairly easy to set up a template that you and your fellow developers can use to formalize the PR process. Github has very specific and well documented ways of doing this. See PR Templates. A favorite, simple template of mine:
## What?## Why?## How?## Testing?## Screen Shots (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 product.
The What
Explain the changes you’ve made. Nothing fancy and you don’t have to get to technical, yet. Just explicit prose on your net change will do. At a high level, this is where you let the reviewer know the overall effect of this PR. Reference a ticket in your issue tracker if appropriate, but by all means, don’t just reference the ticket. Explain what the change is and then and only then, reference the ticket. Reviewers want to spend more time on the changes themselves and less on chasing down a ticket in an issue tracker, especially if the chasing involves multiple layers of authentication. Again, be explicit.
Here’s a few examples of useless, and time-wasting “what” explanations:
## What?
Support for #JIRA-123## What?
Added table to support #JIRA-567## What?
See the subject.
That last example is a particularly annoying one that I see from time to time. I feel manipulated when I see that.
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, 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
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. Almost self-explanatory. Except, it completely changes the nature of the libraries used in your app. Probably worth a comment.
These are poor examples of “why” explanations:
## Why?
See #JIRA-1234## 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-1234 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
The diffs that show up in a PR tell most of the story of the “how”, but make sure you draw attention to the significant design decisions. You decided to write a recursive method instead of a loop, point out the merits of this.
Bad:
## How?
Wrote migration and model.No shizzle. I can see the diff! Help me out here.
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.The Testing
It goes without saying that trying to merge code without accompanying tests will get you automatically flagged for failure to do so. 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. Let the reviewer know how you tested them in case you can’t check in tests. Alternatively, you can show them how to test it locally. Show results of tests you’ve run in this section if you haven’t included any actual tests.
Let the reviewer also know if some edge cases were not tested. Why? And how likely are those edge cases to occur?
The Screen Shots
Some of my favorite reviews include UI changes where the results don’t need too much explanation, just a simple screenshot of the before and after or of the production vs your local development view.
Even backend code can benefit from a screenshot of the net change. An example would be the result from a CLI tool after you actuate 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 perfect spot to put a snapshot of that result.
In 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. Remember, this is markdown and you can get creative. Here’s how:
<details>
<summary>Terraform Plan</summary>
## After running plan: </details>
Anything Else?
You may want to delve into possible architecture changes or tech 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. Call it out:
## 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 comment 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-1234 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.## Screen Shots (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. I know from this comment 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.
If you remove the optional stuff, you still have a pretty concise comment, that is not too long to read. It also reads 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-1234 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.
A Word On Cleanup
The difference between a PR that is clean and easy to follow and one that includes all of the commits and comments you’ve made up to this event, including the thoughts to yourself you may want to keep in an inside voice, is a squash (not the vegetable).
Squashing helps me clean up the timeline as well as keep my PRs from leaking below the fold. You can drop all of the “WIP” commits and flatten the PR using this method. Here’s a good how-to on squashing.
Going Too Far
For those of us who are a bit on the wordy side, be careful of going too far in your comment. I’ve seen some PRs that go well beyond what is necessary to get the review framed. They’ll include paths of executions, truth tables, you name it. This is too much to read and 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 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. Like I tell my 3 year-old when he’s shoving pizza in his mouth, “Tiny bites, please”.
We’re Only Human
Keep it stupid simple, or simple stupid. You are communicating with a person, not writing code all over again. Short, concise and descriptive PRs get reviewer juices flowing, and ultimately move the process along.
