Structuring Pull Requests

For context, here's a quick snapshot of how the development process works in my current team:

• Engineer develops feature branch on local machine;
• Engineer pushes to GitHub and opens a pull request (PR);
• Code owners are automatically notified that there's a PR for review;
• Meanwhile, our CI pipeline kicks in and runs tasks like checking the linting, that tests are passing and internationalisation files are all complete;
• Upon a reviewer's approval, the feature branch can be merged onto the staging branch;
• Staging app is then deployed for internal QA; etc etc.

The bit I want to talk about today is pull requests. My day to day job involves not just lots of opening of PRs, but also the reviewing of other engineers' PRs. I've never had formal guidance on how to do an effective review, but think it's a hugely important part of the deployment process. Because this is a manual step requiring other humans, not setting this step up effectively can really result in lengthy delays on code deployment, which isn't great from a developer experience POV, nor the business.

Through constant reflection on ways to speed up this process whilst ensuring code review quality remains high, I've put together a list of tips on how to structure pull requests. Code reviews involves 2 parties, with the reviewer on one hand, and the engineer who opened the PR on the other. I've chosen to write this blog post from the point of view of the individual asking for the review, because they're the first step in the process. A well structured pull request goes a long way towards helping the reviewer focus on the right things, and nudging them to display the behaviour you want as a reviewee (not a real word, but let's roll with it!). 🎲

The logical place to start thinking about this was thinking about what I want when requesting for a review. This tends to be some mixture of:

• How to make my code more concise and readable;
• Potential performance leaks and suggestions on how to improve efficiency;
• Pointing me to other places where a particular concept / method / constant already exists and could potentially be reused;
• Confirmation that it works on "their machine", not just "my machine";
• That the UI displayed on their device matches the designs; and
• Other tests that might be useful to include;

On the flip side, it's never really about things like linting or code style. The reason why it shouldn't be about linting, is because tools like ESLint and Prettier exist. Teams should decide on a set of rules for everyone to adhere to and codify that in their config files, along with making it one of the automated checks in their CI pipeline.

I also don't really appreciate review comments on writing code in a different style, without there being a clear, tangible reason. If it's just the way the reviewer prefers to write it, I'm sorry, but that's not a good enough reason. If it's something that's important to standardise across the team, codify it as a rule in the team's linter config.

Alright, so back to what I do want from my reviewers - how do I nudge them in the direction I want them to take? Considering that the PR review itself is the main form of communication you have with them, use it to focus their attention and make the review process as easy as possible for them. Here are some ideas of items I include, to allow my reviewers to be as focussed as possible, on the most high value items.

Tips on how to structure your pull requests (PRs)

Keep your PRs small

This is a cornerstone tip. The smaller your PRs, the easier it is to review and thus, the higher the quality of that review. A rough rule of thumb is under 200 lines of code changes. And yes, I know it's not always easy, but try. If the ticket you're working on doesn't lend itself to a small PR, think about breaking it down into smaller sub-tickets before you start coding. This takes practice and experience. I'm still working on this myself, but it does get easier and more obvious over time.

If you really cannot break it into smaller tickets, create a base feature branch, and continue to branch off of that as you logically build complexity upon your code. The reviewer can at least then review your large feature in logical layers.

Make your PR title meaningful

Fill in a title that actually reflects what your feature is about. Don't just use what GitHub autofills in for you, which defaults to your branch name. This allows your reviewer to know at a glance, what your feature is trying to achieve. The added benefit is a clearer git history, for if you ever need to write release notes or rollback to a previous commit.

Add detailed description notes

The more context you're able to provide, the easier it is for the reviewer to try to put themselves in your coding shoes. What I tend to do is to format the description into sub-headers and bullet points. This makes it easy for the reviewer to consume, and also "trains" them on how to get the information they need, since my PRs use the same format each time.

Some items to think about including:

• What your PR does, and perhaps intentionally, does not do.
• Links to the feature requirements and design specs. This allows reviewers to know what they are assessing against.
• Your reasoning for why you decided to do it in a certain way. If it's a simple feature, then I leave this out.
• If the code changes are spread out across a large number of files, try to make notes on the natural order or file groupings a reviewer should take, so that it makes sense. GitHub lists files alphabetically which isn't terribly helpful at the best of times.
• A section that specifically highlights part of the code that I'd like them to focus on because I'd like a second pair of eyes to verify methodology etc.
• I sometimes also include specific asks from my reviewer, for example, asking them to check Android specific hardware features, or specifically getting them to check that frontend effects are reflected in the backend database.
• If it's a UI change, including screenshots and GIFs of the feature in action.

Note that you shouldn't really have to explain what your code is trying to do - this should be clear from how you've written the code. If needed, add an in-code comment instead.

Write commit messages that reflect what has changed with each commit

I was told during my coding bootcamp to keep my commits tight - if you ever have to use the word "and" in your commit message, you've probably waited too long to commit. 😂 I'll put my hand up and say that I'm sometimes guilty of trying to rush out my commit messages when I'm in the flow, but writing solid messages can be a huge help to reviewers trying to make sense of step changes

Additional tips

Set up a PR template for your code repository

We use Github to store our code repo. GitHub allows you to define a PR template that automatically populates the text area when a PR is first opened by the engineer. Here are the GitHub instructions if you're interested.

I set up a template for our team that includes checklist items for both the reviewer and the reviewee, which serves as helpful reminders. This includes things like devices the engineer has tested their code on, and whether analytics has been accounted for.

Set up Codeowners for your code repository

Again, this is for GitHub, with more details provided here. This allows for the automatic setting of PR reviewers (based on certain rules), whenever a new PR is opened.

Track your team's PR stats

There are ways of tracking things like size of PRs and time to review. Tools like this also allow you to gamify the PR review process.