Creating Effective Pull Requests

Tags

Telescope with sunset background over a beach

There are many blog posts out there that provide great tips about which information you should provide when opening a pull request (AKA “PR”). However, the context of most of those articles usually only concerns “the here and now.” I’ve noticed that they tend to neglect the extended life of the pull request once merged and closed. One question that should always be asked is, “What value does the information contained within the pull request provide to future readers?”

Opening The Pull Request

Our scenario starts out with a familiar process in the life of a developer. After hours/days/weeks of working on a new feature or bug fix, you’re done and ready to move on to something different. The only thing that stands in the way of introducing your changes is creating a pull request in hopes of getting them merged.

This can go down in one of two ways:

1) You quickly open a pull request, maybe add a line or two to the description, possibly leaving the title alone, and submit it for approval. You don’t want to spend a lot of time on what is possibly a really small change, or what you would consider to be a self-explanatory feature with an adequate amount of inline comments.

or

2) You open a pull request and provide as much context/info as you can: an overview of changes, testing notes, links, screenshots, etc.

Two buttons meme with man sweating over making a decision to press a button to make a pull request with no information or a pull request with lots of information

Most pull requests fall somewhere between these two examples, which admittedly come off as extreme, polar opposites. However, I’d implore you to consider leaning closer towards the second choice whenever possible—more on that in a bit.

Anatomy of a Pull Request

First, before we dive into the why, let’s look at what comprises a pull request:

Title

Depending on the system you use, when you create a pull request, your first commit message or branch name may become the default title. It is possible you have some guidelines as to what this title should be, or it could be inconsistent across many pull requests.

Description

This is where you’ll spend most of your time, and I’ll go into greater detail on that in a moment. For now, just know that the pull request description is where you can let the reviewer know what changes were made, in addition to other information that will help their code review.

Metadata

Depending on where you are storing your code, you may be able to add additional metadata to your pull request. Labels, associated projects, milestones, and linked issues are some of the more common items (especially on GitHub). They may not add any immediate value to your pull request, but they can help a future user find your pull request.

Now that we know what makes up a pull request, let’s understand why this information needs to continue providing value after the merge.

Why you may need to review old pull requests

Consider a repo that has been around for a while (3-5 years, as an example). Teams change, features evolve, knowledge transfers, but to what degree/depth? The what/why/how of something you implement today might not be immediately known later on. Or maybe the one person who implemented these changes and could answer questions you have about it no longer works for your company.

A scenario you might find yourself in is looking over a block of code or tracing a logic flow across multiple files within the app. There might be either a branching block, or possibly some set of checks, that you’re not exactly sure why they’re there, or if they represent old logic that never fully got cleared out/no longer applies to your app today.

If there aren’t any inline code comments, or the comments do not provide enough information, you can always check the git blame/history for that section to see who made those changes, when, and with which pull requests. Having descriptive pull request titles can make this investigation a lot easier.

Or maybe you use a code editor that shows you this information inline without having to leave your editor. You might also be able to work backward through the history of the file so you can easily see what changed over time. But you might still need to check out the original pull request for more information if you don’t have all of the information and context that you need.

Once again, though, we are potentially helpless to fully understand these changes, or know if any changes to this one section of code will unexpectedly impact some other part of the app. Our next move is to make a note of the pull request number (or numbers), and head over to the repo to look at closed pull requests.

Why effective pull requests matter

Let’s assume for a moment that we have a block of code that we need to make changes to. The current history on this code block spans two pull requests — the initial pull request, and one that made a logic change to it. You recall there may have been an issue with this section in the past, and want to do some research to make sure you don’t introduce any regressions.

The latest pull request you come across for this section isn’t very helpful: GitHub pull request that only has the branch name as the title and a vague two-word description

There is not much information provided in this pull request, presumably going on the assumption that the reviewer will know the reasoning, context, and history behind the changes. Or maybe the reason for not providing much detail is a need to merge quickly with the intention of coming back later to update. Either way, we have no idea which ticket this change may belong to (if any), nothing with regards to why this change was made, or any notes on how to test this change.

It is quite possible that the developer who made this change has since left the organization, so you can’t ask them. Or maybe the change was from 2 years ago, and no one on your team remembers any details from that long ago other than it was a super complicated bug to identify the source of.

Moving on, you might be able to infer some information from the original pull request: GitHub pull request with issue tracker id and title, brief description, and metadata labels for pull request classification and milestone

This example is a little better, but still not as helpful as we would like. While we have a reference to the issue tracker ticket, and which milestone this change was a part of, there isn’t really much more to go on. For more information, we would have to look at the ticket in the issue tracker system in hopes that there is more detail there, or possibly follow-up with the original developer or project manager to see what they can remember.

Oversharing is caring

This brings us to the question at hand: what information should a pull request have to withstand the test of time and help in this situation? The pull request you create today should be useful weeks, months, even years into the future. Here are some of my suggestions:

Title

A useful title would be one that matches what the title of the ticket would be in your issue tracking system, or at the very least, descriptive enough to communicate what changes were made in a single line.

Pull request titles are viewable in list form (think open or closed pull request lists), possibly used in auto-generated changelogs, etc., so they’re not just for reviewing your changes. jleja/my-feature or First commit will force people to open a pull request just to see what it’s about. A more helpful suggestion would be to make it match titles you would find in your tracking software ([FEAT-1234] Upgrade Ember to 3.21), or if that doesn’t work for your project, use a more helpful title that tells what the pull request is about without having to open it (Replace Bootstrap with Tailwind, as an example).

I highly recommend that your title is not truncated with an ellipsis to make it more effective outside of just reviewing your changes (like when reviewing lists of open/closed pull requests or pull requests included in a version build). If your pull request is using your first commit message, and that message is sufficient as a helpful title, then it’s easy enough to let a system like GitHub split the sentence up and continue it within the pull request description. (Example: [FEAT-1234] When presented with an incom...)

On a side note, this title would also make a great first commit message in your branch. If you use the “squash and merge” method of merging your changes into main, it will use that first commit message as the one displayed in the commit history. This will make that list just as useful as the closed pull request list. This approach fits best when your pull request can be associated with an issue tracking system ticket.

Description

This is another opportunity to shine. The documentation you provide here could be just as, if not more, useful than inline code comments. Items such as a clickable link back to the issue ticket in your tracking system, description of what your pull request solves, notable change log comments, testing notes, before/after screenshots, etc. will provide an extra layer of documentation should you, or someone else, ever need to refer to them.

Your pull request description can also be structured in a way that, once you’re finished, you can copy and paste as a comment on the ticket itself. This information is typically requested in that system as a way to let QA know how to confirm your changes work. So you can write it all out once and use that information in multiple places, saving time and effort while providing consistency between disconnected systems.

I hear the argument all the time that everything that we need to know about the pull request is in the ticketing system, so just look there. Or maybe the description in a bug ticket includes a reproduction of how to recreate the bug, so why duplicate that information.

But this kind of thinking fails to address lack of access. Other developers may not have the security privileges to your issue in the tracking system. QA folks most likely won’t have access to GitHub, nor do they want to refer to GitHub in order to provide comments elsewhere. By copying your pull request description to your issue tracker, you provide the same information needed by everyone where they have access to it.

The information you provide can be broken up into sections:

Issue Link

Markdown example: [FEAT-1234 - Customize adapter headers](https://tracker.example.com/browse/1234)

Provide a clickable link that takes you to the issue page if your system is web-enabled, or auto-launch the issue tracker app to that ticket. Instead of having to open the page/system and manually search for that issue ID, you make things worlds easier for your reviewers by letting them just click a link.

Change Description

A short paragraph or two describing the problem, solution, and what you found out along the way would be useful and puts your code changes into context, explains why you did what you did, etc. If a simple bullet list of changes you’ve made works better, then this section can be omitted. This could also be a good place to link to any other related issue tickets or pull requests that are blocking this from being merged or will be where known issues are taken care of not covered by your code. Optional but recommended if not providing a list of changes.

Changes

A bullet list of things you’ve done in this pull request. Not the same as listing all of your commit messages, especially if they’re things like “blah” or “maybe this will work”. No, they’re more just an overview of what the pull request contains. Optional but recommended if not providing a change description.

Environments

What environments can this change be tested in? Do you have separate QA/UAT servers? Any particular build flags needed if testing locally? If none of this information applies to your project, or if this information is better served by providing links, then feel free to omit.

Links

Assuming this is a web app, are there links you can provide to specific pages where the problem can either be reproduced or confirmed as fixed? This section can be optional if you aren’t able to provide direct links to pages. If there are no specific links to use, then you can omit this section.

Testing Steps

Most issues should be able to be tested by someone before being merged and going to QA/UAT. Nothing is more frustrating than reviewing code that you want to test to confirm that the issue is resolved and no new regressions are found, only to find out that how to test isn’t as obvious as it could be.

While a little more time consuming, I like to strongly suggest that you assume the person reviewing your code has no to very little familiarity with your app. Sure, the person reviewing your code is an expert with it, however, if someone from the distant future is reading this pull request, you can’t assume that level of familiarity or expertise.

There are some funny videos out there of parents asking their kids to write down step-by-step how to complete a particular task (like making a peanut butter and jelly sandwich). The parent follows the steps exactly without any inference, and the kids continue to revise their instructions until the end result is what was expected.

It seems comical, but if your testing instructions read like a testing script, then it forces you to do a few things:

  • Test your changes in an end-to-end scenario
  • Allows you a chance to fix any related bugs along the way that you didn’t notice before
  • Provide documentation for future readers in the event they also need to test that your changes still work (or at least have an idea of what your changes relate to)

I’ve actually come across some bugs in my own code changes or realized that my solution didn’t quite fix everything, simply by running through and creating a test script of all the things the reviewer should look at.

Screenshots

Everything up until now has just been words. Descriptive, helpful, beautiful words. Bob Ross would consider them the happy little trees of the pull request world. But, sometimes, words aren’t enough. That’s where screenshots come in!

At the very least, include screenshots of what you’ve changed in action. If this is a new feature, then various screenshots of the new feature in action will help show the reviewers what you’ve added. If this is a bug fix/enhancement/etc. before and after screenshots side-by-side can provide an easy way to compare changes.

Here’s an example of what your Markdown code would look like to accomplish this:

|Before|After|
|---|---|
|![before](/path/to/before.png)|![after](/path/to/after.png)|

Rendered example:

image showing rendered markdown table with Before/After headers, and a before/after image in each corresponding column

Pulling it all together

Given all of the provided information, how could we best structure our uninformative example into a pull request that could be beneficial for future readers? Behold:

GitHub pull request that has an informative title, detailed change description, testing links, and testing steps outlined

Any links to external documents would presumably have enough information for the reviewer to set their environment up to perform the necessary memory heap snapshots. In any case, you can at least see a lot more information about a one-line change that was made to resolve a big issue. I’ve also used the sidebar metadata fields to add tags to indicate that this bug was ready for review, and which milestone the change went against.

Side note: The example I used is based on Sergio Arbeo’s Fast Flood presentation at EmberConf 2020, so feel free to check that out if you’d like to learn more about the example I chose to highlight.

Creating a template

One thing you can do to ensure pull request consistency within your project is to determine what a good pull request is with your team, then turn that model into a pull request template! GitHub easily lets you do this, as other systems may as well.

Want to quickly get started? Here’s how you can easily do that with a sample template:

  1. Create a file called PULL_REQUEST_TEMPLATE.md somewhere in your project. It can be in the visible root of your project, in a /docs folder, or in a hidden /.github folder in your project root.

  2. Paste the contents below into that file, and modify to fit your needs:

## Issue
[FEAT-1234 - Issue Title](https://tracker.example.com/browse/1234)

## Change Description
Description of the problem and how this PR solves it.

## Changes
* ...
* ...
...

## Environments
*QA, UAT, etc are important to suggest to your PR reviewers so they know where to test*

* QA
* UAT  

## Testing Steps
*scripted steps for the reviewer to follow that ensures they know how to test your changes*

1.
2.
3.
...

## Testing URLs
*example URLs that your reviewer can use so they know which pages to check your changes on*
- https://www.example.com
- https://www.example.com/page/1234

### Screenshots
|Before|After|
|---|---|
|before_image|after_image|

Conclusion

The above suggestions may seem like a lot of steps, requiring more effort than what you are willing to ensure just to get your code merged. The thing to keep in mind is that you’re part of a team, and as part of that team, you share the responsibility of ensuring that your code is well-documented and maintainable.

After you get in the habit of consistently providing good information in your pull requests, you will know when to provide a particular section of information, and when it is safe to omit it. What I have detailed are simply suggestions that I think would work for most projects, and have worked in many projects I’ve contributed to over the years. It’s now up to you and your team to determine what works best and what would be most helpful.

DockYard is a digital product consultancy specializing in user-centered web application design and development. Our collaborative team of product strategists help clients to better understand the people they serve. We use future-forward technology and design thinking to transform those insights into impactful, inclusive, and reliable web experiences. DockYard provides professional services in strategy, user experience, design, and full-stack engineering using Ember.js, React.js, Ruby, and Elixir. From ideation to delivery, we empower ambitious product teams to build for the future.