Crafting the Perfect Pull Request

It’s become de facto in our day-to-day development to work on new features via a branch and open pull requests (or merge requests for GitLab users) to merge any changes we’ve made on our development branches into a production or deployment branch.

As a crafter of some seriously awful pull requests in my time, I figured it was worth sharing some handy reminders that will help you avoid the most grievous of mistakes that I’ve succumbed to over the years.

Following these guidelines can drastically reduce the delays for shipping code that can invariably drag on for a week+. It requires slightly more effort and thought when crafting your PRs, but it leans on the principle of slow is smooth, smooth is fast.

Don’t Be Selfish - Keeping Lines of Code Changed To A Minimum

This is likely the most important point in this list. Failing to do this can result in resentment from your colleagues for making them trundle through hordes of files trying to ensure the soundness of the work.

Reflecting back on some of my previous PRs, I realise now that there is an element of selfishness involved when you construct large PRs with reckless abandon. We don’t generally want to be selfish in our work so splitting these things up into smaller, more digestible chunks makes it far easier for your colleagues to do proper reviews without putting too much on them.

Splitting up your larger pull requests also effectively ties into the mantra mentioned above of slow is smooth, smooth is fast. Incrementally releasing bigger changes allows you to quickly validate smaller chunks in production which can ultimately reduce the need for rollbacks.

Bonus: Improved Collaboration and Code Quality

We’ve all been there. Swamped under a mountain of tickets and high-pressure deadlines to ship features. A colleague in your team lands a huge PR that’s urgent and needs a review.

The temptation to give these things cursory glances and press Approve is high and it’s a trap that I’ve seen a heck of a lot of developers fall into.

This practice ultimately gives rise to an increase in poor quality code that goes into your systems. So by splitting these PRs up, and pushing back on having to review these larger PRs, we invite more collaboration across the team as more developers are likely to spend time really going in depth into the details.

We can also more easily see that code has appropriate test coverage and we’re doing all the things that really prove us to be ’engineers’ as opposed to coders that turn out ticket after ticket.

Having A Bulletproof Pull Request Template

Pull Request templates are a fantastic way to consistently remind yourself what needs to be done prior to a pull request being ready for review.

You can add one of these to your repository by creating a file called .github/pull_request_template.md, assuming you’re on GitHub or .gitlab/merge_request_templates/mytemplate.md if you’re working on GitLab.

## What Did You Change?
__summarize what you intend to change in this PR__

## Why Did You Change It?
__why are you changing it__

## Potential Customer Impact
__Outline what the potential customer impact is of this change__

## Jira Ticket
__What jira ticket is this work associated with__

## Testing approach
__How did you test this change?__

## Pre-Review Checklist

- [ ] I've kept the code changes small
- [ ] I've ensured happy, sad and edge case tests are in place
- [ ] I've ensured the commit messages are descriptive and well formed

As you can see from the above, the questions are framed in such a way that it assumes your code changes will have tests and asks you to describe them.

Even being able to outline potential customer impact in the pull request description itself is invaluable during those inevitable production incidents where you are scrambling to try and find the root cause across a fleet of services.

Follow Good Commit Practices

We’ve all been there at times when you’re battling a particularly annoying bug and running it through CI numerous times to try and narrow down on it.

You’re commit messages get sloppier and sloppier to the point where they resemble sounds like urgh and grr.

This is all good for general development, but as soon as you’ve squashed that mischievous bug and landed that new feature with all your shiny tests, it’s time to give those commit messages some good old TLC.

I’ve worked with a few exceptional engineers in my time, and one thing that has stood out to me about their process is how much time and energy they invest in their commits. Dan Carley is one such example of an exceptional engineer and has really engrained good commit practices into both myself and my partner (we did work at the same company for a few years).

I’m not the first person to blog about Dan’s commit practices, one of his former colleagues from the Government Digital Service has an excellent post on this subject here - My favourite Git Commit.

If we could all aspire to follow Dan’s example, software engineering as a whole would be a far better place.

Test Coverage - The Good, The Bad, and the Ugly

It would be easy for me to put out a sweeping statement such as “All code changes should be tested no matter what”. In reality, this isn’t always possible or even sensible depending on the circumstances around the code change.

However, not having tests alongside a pull request is one sure-fire way for a pull request review to take longer. As engineer’s we should continue asking the question of “why has this pull request not got any associdated tests around it?” at every opportunity to ensure our own standards are being upheld when it comes to delivering high-quality work.

When I look at a pull request for the first time, I tend to navigate down to the tests that have been updated as part of it and ask myself - “do these tests give me enough confidence here?”. If the answer is yes, then typically the pull request review process goes far smoother and we’re able to land that particular code change faster without wasting time going back and forth.

Functionally Scoping Pull Requests

This is an important one, and there is some nuance to it. I’m an engineer who is guilty in the past of working on a ticket and then realising “oh, I could just quickly fix this section of the code here… oh and here as well…” to the point where my PRs become unrecognisable amalgamous mess.

It’ll implement the acceptance criteria marked out in the ticket, but I’ve perhaps drifted a little too far from the course with my desire to tidy things up as I go.

For prospective employers reading this, I have actively worked on this aspect, but did feel it was very much worth mentioning in this article so that other developers out there don’t fall into the same traps that I used to.

And, for the record, this isn’t me saying “don’t follow the boy/girl scount rule of leaving things better than you found them”. It’s instead trying to ensure that when you do consider doing some tidy-ups alongside the code you’re touching, you don’t overdo it.

If anything, I’d very much encourage tidying up, but perhaps splitting this into a separate PR that goes in prior to the feature work would be a far better option.

For those interested, this approach is something that Kent Beck suggests in his book - Tidy First? which I highly recommend. It’s a short read, but I did find it valuable when thinking about how I approach things from a more strategic point of view.

Wrapping it Up

I hope you found this article useful! I’ll be continuing to add to it and tweak it in the months and years to come as I find my approach evolves over time or new things come to light!