Having a Healthy Pull Request Process for Teams

A Good Pull Request Process is Crucial to Team Health
As an Author: Make PRs Easier to Review
As a Reviewer: Make Reviews Easier for the Author
As a Team: Agree on PR standards
- Prioritize Reviews
  - Get team agreement on ticket and project level priorities
  - Decide on a Reasonable Timespan and Notifications for Reviewing PRs
- Decide what should be blocking and what should be non-blocking
The Result: A more healthy PR experience

A Good Pull Request Process is Crucial to Team Health

Pull requests help teams manage risk in their services by allowing multiple team members to have input on the code that goes into production.

When done well, a good pull request process encourages crucial and productive feedback. Team members collaborate on improving their services, and all team members feel heard. When I feel heard and supported by my team, I feel more motivated and less hesitant to knock things out, knowing that with the code I write, good or bad (we all struggle to find the “right design” at times), I will get the feedback from my team necessary to move ahead.

When done poorly, a bad pull request process encourages unproductive feedback, making it a battle of “who has the right opinions”. This can make team members feel exhausted and unheard, and this can lead to hesitation to open PRs or even cause members to leave the team or community.

Given how important PRs are to our ability to ship features and fix bugs, it’s crucial that our interactions with PRs are both healthy and productive.

There are three important roles to the PR process, and therefore, three areas where we can make PRs a happier, healthier place for our teams:

The Author: as an author of a PR, we have responsibility and room to make sure our PR is clear and approachable to our team members so that they can give a confident review.
The Reviewer: As the reviewer of a PR, we have the responsibility to unblock the work of the author. We do this by approving the pull request, or by requesting needed changes and providing explicit feedback when necessary. No matter which way the review goes, it’s crucial that the review is timely so that we don’t block and frustrate our team members.
The Team Itself: Good PR practices are helpful at an individual level, but to be most effective, the team should agree on and adopt a set of standards that the team can collectively hold PRs to.

As an Author: Make PRs Easier to Review

The first role of the author is to make changes to a codebase to improve it.

The second role of the author is to make those changes in an understandable way and in agreement with the reviewers.

To do that well, I think that it’s important to ask, “As the author, have I made my PR easy to review?”, and make the necessary changes to confidently say “Yes”.

Here are some ways we can do that.

Make the PR Description Clear and Digestible

Pieces to a good PR description

First off: Give the pull request a description. Not providing a description puts the work of figuring out what the PR is doing on the reviewer, and that’s not their job.

The description should answer the question, “What is the purpose of this code?”

Here are ways that we can answer that question:

Provide context to what brought about the Pull Request: If it was a ticket, provide a linked title of the ticket. If it was a Slack conversation or comment from elsewhere, provide a short summary that includes the relevant details of that conversation. This helps the reviewer frame the problem and find additional context if they want to.
Provide a summary of the problem the Pull Request is solving: I always feel like it’s good to summarize the specific problem that the pull request is solving. It’s a quick way to get the reviewer up to speed. It’s not enough to give the reviewer a link to the ticket because that puts the work of digging up the problem onto the reviewer.
Provide a summary of the solution that the Pull Request provides: Like the problem, it is always helpful to provide a short summary of the solution as well as any reasoning for why the specific solution was chosen if there are several obvious approaches.

By providing initial context and a problem/solution statement, we help the reviewer to either understand the decisions we made in the PR or see things that we may have not seen. Both are great.

Make it Digestible 🌯

Context is great, too much context (or a PR summary written in a college essay format) is taxing for the reviewer.

The description should have a short summary of the PR and use Plain Language so that the reviewer can understand what’s going on without having to pull up a list of business acronyms or find the dictionary.

This doesn’t mean leaving out context. It means that we make the important stuff clear, upfront, and easy to read (almost like a TLDR).

If additional info is helpful to the reviewer, we can provide more detail in another section like “Additional Information”, with links to relevant documentation and discussions.

Explain Unexpected Changes

There are times when a line of code looks weird, but it’s required. Having a comment there to explain why it’s necessary typically starts a good conversation.

Leaving Comments on Peculiar lines — Anticipate and explain areas of code that a teammate might find confusing.

As an author, we can foresee questions a reviewer might ask because we may have asked them ourselves when writing the code.

Providing an answer before the question is asked both saves time for the reviewer and builds trust with them.

Keep the Size of Pull Requests Small (When Possible)

Pull requests are like the classes in our code: when one is doing a lot of things, it can be hard to comprehend and understand its purpose.

I find myself dealing with large PRs like a textbook, paying extra close attention to each line. This is very taxing and requires significant time and brain energy, and I sometimes find myself putting off this kind of review for a later time.

For some people, they’ve mentioned that when a PR is too big, they’ll skim over and just approve, trusting that the author knows what they’re doing. Skim reviews go against the point of PR reviews, and making them common is a bad sign of the team’s process.

To prevent large PRs, we have to be intentional on making smaller, clear PRs. Tickets and PRs don’t require a 1:1 relationship; if there’s a clear way to break down a ticket into multiple PRs, we should do that.

Smaller pull requests, focused on a small set of changes, are much easier to review than a large one changing many things. Small PRs are also safer; bugs are easier to pinpoint and fix with small changes.

That said, preventing large PRs isn’t always possible. Sometimes, I can’t find a clear way to break up the code into multiple PRs. Other times, my PR starts as a lump of code that I have to shape into something meaningful, and my commits represent that mess; cleaning that up may not always be worth my and my team’s time.

Knowing that, large PRs should be far and few between, for the reviewer’s sake.

Breaking PRs Into Parts

Here are a couple of approaches I use to break down a large ticket of work into PRs.

Scope out changes with logs or metrics

Say I have a ticket to listen to a new behavior (a somewhat-trivial change) and to make a new decision based on that behavior (a somewhat complex, heavily tested change), I can break this work into two PRs:

Log the behavior: The first PR looks for the new behavior and logs or sends a metric when it sees it. These kinds of PRs provide business value by showing in realtime how often we can expect that behavior to happen.
Make the decision: The second PR takes action on that behavior. By having this separated out and well tested, it’s easier for reviewers to see code changes solely related to the action.

By breaking the ticket into multiple PRs, I’m able to provide value quicker (I can answer “How many times do we expect to see this behavior?” after the first PR), and I’m also able to pass on the second part to another teammate if I need to step away from the ticket.

Separate refactors from behavior changes

Another way to separate code into PRs is to focus one PR on refactors and one PR on behavior changes.

This helps the reviewer to not have to separate “this code is different but should work the same” from “this code is different and will affect the behavior of our service” in their heads.

To misquote Kent Beck:

for each desired change, make the change easy (warning: this may be hard (put it in the first PR)), then make the easy change (put it in the second PR) - Kent Beck, sorta

Make Sure the Team Knows the Problem Before Opening a PR to Address It

Whether it’s a refactor or a bug fix, I find that when my teammates don’t have the context of the problem ahead of time, opening a PR and asking for reviews out of thin air tends to create confusion.

In cases like this, I have learned to always communicate a problem with the team before trying to solve it.

Sometimes the “problem” isn’t actually a problem, like expected but not-well-documented behavior.

Communicate a Problem Before Addressing it — Communicate the problem with the team before fully addressing it.

In the case that it is a real problem, communicating the problem first allows the team to have input into the solution, and they will also have the context to the solution you’re intending to provide.

When prioritizing the communication, input, and team agreement for a problem and its solution, I find that the subsequent PR often gets a swift approval and recognition for the diligence and communication I initially hoped for.

Listen to and Take in the Feedback Received on PRs

Despite doing all of the things above, we will still get feedback and blockers on our PRs. That’s a good thing. The recommendations above are to weed out the unnecessary feedback so that we can hopefully focus on necessary changes.

Given that, I still know the mindset that’s hard to lose: “I’ve spent significant time thinking through this problem and providing a solution. Why can’t you just see it for what it is and approve?”

That mindset is understandable but wrong: the purpose of reviews is to see the PR in another pair of eyes, not to see it in the same way we wrote it.

This doesn’t mean that we have to make every change requested. But it does mean understanding the problem that the reviewer is bringing up and addressing it collaboratively.

For example, sometimes a reviewer may have a blocking question that, after a good discussion, becomes non-blocking. This may mean we decide not to make the change or we decide to make the change in a future PR.

Delegate non-critical changes to a future PR so that the working code as-is can be merged and marked as done. This has been a lifesaver on many tickets I have worked on (Thank you B.J. Allen for teaching me this).

Other times, some bugs or code changes absolutely need to be addressed, and this is why we do pull requests. 😄

Summary as a PR Author

The tips I have learned and provided here all go towards answering the question, “How can I make this PR easy to review?” We should make sure that we are providing our team members with the information they need to give reasonable feedback.

I think doing so will set a standard in the team for members to follow and improve on.

As a Reviewer: Make Reviews Easier for the Author

When I am a PR reviewer, my responsibilities in a review are:

Unblocking the PR author: The author wants to merge their code changes so they can complete some work. My review is what helps them move forward, whether it’s through approving or requesting changes.
Validate the code: I want to make sure that the code in the PR looks like it is doing what the author intended it to do. This includes ensuring that there is proper testing.
Provide feedback: I am a second pair of eyes for the author’s changes, and I can provide feedback on different approaches that the author may have not considered.

I also want to do this in a reasonable amount of time, so that the author does not have to sit on their work for too long.

To each of the responsibilities above, here are some ways that we can do our part to ensure that the author’s work is successful.

Give an Explicit Approval/Disapproval with Explicit Reasons

It’s important that when we review a PR, we give the author clear next steps. We can start this by providing either an approval or an explicit “Request Changes”, rather than just providing comments.

When Not Approving, Distinguish Which Comments are Blocking

If we “Request Changes”, it’s important to distinguish which comments need to be addressed before we will approve, and which ones are fine not being addressed before merging.

I typically use the term “Blocking Comment” for comments that need to be addressed and “Non-Blocking” for ones that I’d like to see addressed but won’t block the approval on. There are probably clearer terms or a better way to clarify the comments, so I’m open to recommendations. 😄

Distinguish Between Blocking and Non-blocking Comments — Help teammates know what changes/questions need to be addressed by specifying them in the comment.

In the examples above, the “Blocking comment” is for a bug that needs to be addressed. “Addressed” could mean making a code change or discussing why it’s not a bug if that’s the case.

The “Non-Blocking comment” is for a recommendation for how the code could be improved. “Improvements” are highly subjective, and each person may have a different take on how to best improve a section of code. I don’t like blocking someone whose code already works, and that’s why when I recommend them in a PR, I typically give them about a 3/10 on my “give-a-shit” meter for addressing.

Express full thoughts rather than shortening sentences

I find that comments like “this line right?” send the message that I don’t care enough to express my full thought, and yet I expect the author to care enough to respond in some meaningful way.

Because comments like this are missing a lot of context for the author, I find that they can also cause the author to doubt themselves (“What have I missed here? Is it obvious?”) or take a mental defensive stance: “Of course I think “this line right”, otherwise I would not have written it that way.”

The role of the PR author is not to interpret what the reviewer is thinking. In order to be a good PR reviewer, we should adequately communicate the problem that we see so that the author can quickly be on the same page. We can express our thought process in a few ways:

Address what the author is intending to do with the certain code: “This code looks like it’s taking the array of hashes and pulling out the relevant data using .fetch.”
Address our concern with the code: “I think this code will error in the case that a hash in the array is empty. If so, I think it would be good to check for that case before this line or make this code work with an empty hash.”
Address why our concern is relevant: “I think in the specific case of the customer having not purchased items, we can expect the hash to be empty. Looking at our graphs, that happens pretty often.”

All of this helps the author see what we’re thinking right at the beginning.

Know When to Reach Out

As an author, I have experienced the dread of seeing a dozen new comments on a PR. As a reviewer, we can prevent that dread for the author.

When I find myself writing more comments than there are lines of code in a PR, that tells me that I’m not on the same page as the author.

I find it’s helpful to reach out to the author in cases like that and see if a synchronous discussion would be helpful. A video chat or quick Slack DM chat can quickly ground everyone in the problem and give a chance to discuss the solution and share feedback synchronously.

Reach out when it's helpful — Doing a 5-minute call can quickly clear up mass comment confusion.

Reaching out to the author makes for a good opportunity to pair and collaborate, rather than two people playing comment Whack-a-Mole. A good way to round off the chat is provide a summary of the discussion in the PR for the context of other potential reviewiers.

As a Team: Agree on PR standards

I have learned that the team should discuss and decide on the things it cares about for PRs and the things that the team doesn’t care about.

Doing so makes for a more consistent process, which I find gives a certain peace of mind when writing a PR. I know what types of feedback to expect from my teammates and when to expect it. I know where I may be blocked when I make certain decisions and where I’m free to play around with.

Here are a few key areas that I think the team should agree on and document.

Prioritize Reviews

I believe that reviewing a pull request is just as important as writing one.

When the team doesn’t prioritize PR reviews and instead every member focuses on their own PRs, the behavior creates islands of work where each member thinks their work is the most important. This tells me that the team doesn’t agree on what is their highest priority.

Get team agreement on ticket and project level priorities

An effective way to agree on priorities is to discuss as a team “What is our current highest priority?” both at standup and in iteration/sprint planning.

At standup, the priority should be at a ticket level: “We’re focused on this project and the tickets available to work are #123 and #124. Let’s help push those to completion.”

At planning, the priority should be the project level: “We’re focused on this project, so let’s make sure the tickets are up-to-date and ready for the sprint/iteration. We’ll also spend time working on this other project, so let’s do the same for its tickets.”

When the team agrees on which projects and tickets are the priority, I find that PRs for them get the attention they deserve.

Automate Standards When Appropriate 🤖

When the team decides on certain standards that it wants to hold for a PR and those standards are automatable, automation can take the work off of authors and reviewers to enforce the standards.

An example of an automated standard is:

Standard: “We require tests to pass requiring tests to pass before merging.”
Automation: “Use CircleCI to run the test suite for changes PR and require it to pass before allowing it to be merged”

I’ve seen other useful automation for code patterns (Rubocop, etc.), dependency updates, or dependencies with security vulnerabilities.

I’ve also seen non-useful automation that inappropriately blocks PRs or adds more pain than it helps, so automation isn’t always the answer.

I think that it’s most important to ensure that the automation provides value to the team and is not a hindrance.

The Result: A more healthy PR experience

By being more mindful and clear in how we approach our own PRs and the PRs of our teammates, we can make PRs feel like a place for collaboration. They become a place where we can show our work and get real feedback without judgement.

Team members should feel free (or even be encouraged) to push up code that they are hesitant on, so that others can give feedback on how to better approach the problem.

I have opened PRs many times and told my team, “This code works, but I’m not happy with the design.” The feedback I receive often results in a PR that’s completed quicker with code that I feel good about. That kind of collaboration builds momentum in the work around the team’s priorities.

Having a healthy PR process makes for a healthier productive team.

Alex