Pull requests, or PRs, are a functional requirement in any cooperative development environment (and single development environment, in my humble opinion). They provide a well-formed gate between newly-developed code, and stable code that is in a known, good, tested state that any developer can freely use as a baseline for additional feature work. Catching mistakes or unintended behavior by reviewing changes prior to permitting code into the stable branch prevents unintentional deployments of broken behavior, shortens feedback loops on design choices, and encourages collaboration by sharing work done among one’s team.
They can also be a massive pain in the posterior.
You’re diligently working away on your own feature. Then you get that notification in your team’s pull requests channel, or your GitHub notifications, or e-mail, or all three: Time to context switch! Your workflow is disrupted as you tear away from your coding to go review a collaborator’s efforts. Their pull request could be a full work day in the making, with lots of complex changes that sweep across the entire application. Perhaps the PR is not related much at all to your focus, forcing you to internalize another part you’re not familiar with. Others are leaving comments or possibly already marking the PR as Approved, and everyone’s waiting on you to review it. Discussions can emerge within a PR, forcing you to spend even more time in review mode. Then the PR author pushes new changes, resetting everyone’s approvals or notes and sends it all back to square one. The automated build and test processes that take 10 minutes to run start over. All of this swirls in your mind while you know your own obligations are being delayed. What a pain!
We put up with this for the obvious advantages and necessities of pull requests. The pillars that structure an organization’s pull request process, including automation, required reviewers, and the size of the request, all contribute to that dreadful notification ding. Or, it can be an easy-going experience, if proper etiquette is followed to streamline the entire ordeal. The preceding paragraph highlighted some hypothetical pitfalls that stem from poor pull request etiquette. Let’s explore the responsibilities we take on both as reviewers and reviewees, as well as policies that can make PRs a smooth sendoff or a living hell.
Good Etiquette for Pull Request Authors
When you are ready to create a pull request, you have options. You can blithely say to yourself, “I completed my work and now it’s time to throw it to the wolves. That’s why we have a review process in the first place, after all: It’s there to check my work!” It’s a mistake I myself made now and then, long ago, and quickly learned from! Creating a PR (or, in other contexts, sending your work to QA) inescapably creates more work for others. This is okay since the PR process exists for a reason, but as PR authors we are responsible for removing boundaries from the burden we place on our collaborators. Prior to creating a PR, there are steps you can take to ease the burden.
Review Your Work
Whether it took you 10 minutes or 10 days to complete your work, always review it prior to opening a PR. Draft PRs are an excellent tool for this, as you can do all the setup work for your PR, without officially opening it. Review your own PR draft as if it was not your own. Leave comments, push new updates, whatever you need to do. I’ve come across countless silly typos and obvious gaffes in past reviews I’ve done, solely because the author obviously did not take a little time to review their own work prior to asking others to. Now they’re burdened with an incipient re-review as a quick change is needed, as the “Reset” button has to be pushed. If you haven’t self-reviewed before, start, and marvel at what obviously sticks out as you do.
Everything you catch, great or small, speeds up the PR process for any collaborator, showing respect for their time, and saves yourself time having to re-submit.
Run the Automated Builds
A review of one’s work in a PR goes out the window if the PR builds or tests fail. Before announcing your PR to the world, verify the build completes, the tests pass, the code coverage clears, and any other automated checks succeed. Let those tools that you and your team set up do their work before tasking others to take time to review.
Keep Pull Requests Small
Submitting a PR that touches 100 files encourages poor review etiquette. Skimming will happen, things will be missed, or in the worst case, rubber-stamping (i.e. a collaborator instantly mashes that Approve button) will take place. Self-reviews will be discouraged as well, since that review will take time. Any change that requires re-review compounds time spent even further. When developing a new feature that is complex, try to break it down into manageable chunks of work. Submitting large PRs encourages others to do the same, which further exacerbates the pain points of PRs.
If there’s a concern about submitting partially-completed work that would break the existing application in some area, there are means to help promote separation. Make use of Feature Flags to keep the feature offline. Have intermediate work not flow immediately from a feature branch into the stable branch (whether that’s organization-wide, or just managing your own feature branch, that has sub-branches), and chunk the PRs up that way. Finally, you can structure your changes so that you can complete intermediate work that simply isn’t called or activated (if Feature Flags aren’t available to you), by building out classes that aren’t invoked or are temporarily shortcutted to no-op.
Keep your PRs small to the best of your ability, but it’s not always in an your control to do this, as we’ll see next.
Why Pull Requests Become Large
There are several ways pull requests get unwieldy at a systemic level. Even in the best of environments, you’ll have the occasional super-massive PR that’s unavoidable and necessary to do. This section doesn’t address the rare one-off cases of large PRs, but rather collaborations that see them frequently, or are even encouraged.
Author Inexperience
Whether it’s a developer new to the application or domain they’re working in, or just being new altogether, an inexperienced developer can end up submitting large PRs on the regular. They may see it as normal if they’re surrounded by other collaborators doing the same, or they just don’t know any better. There is no quick fix or “just follow these good etiquette tips” for this one: Experience comes with time and practice. Ideally, a newbie is surrounded by positive feedback from collaborators who can set things right and teach (by example!) why large PRs aren’t great, and how they can work towards smaller, more efficient PRs in the future.
The Codebase is Difficult to Change
Not every application has a fantastic codebase. I’ll give you a moment to pick up the monocle that just popped off your face. 🧐
Sometimes a codebase just flat-out won’t let you submit small pull requests. The application could be a big ball of mud where even the tiniest feature request or bug fix requires sweeping changes. What may seem an innocuous “fix this button” or “add a boolean to a domain object” cascades across all layers. The codebase encourages touching stuff everywhere because it’s not in the best shape, or is built off decades of legacy.
The fix, of course, is to gradually make the application better. Time is forever a factor in the wonderful world of software development, but even if you could just adhere to practices like the Boy Scout Rule, ideally you can chip away at the big ball of mud and eventually whittle this problem away. The more collaborators work towards this goal, the more time is saved all around (not just in PRs, but in developing new features).
Time
Alluded to in the previous segment, you might not have the time to properly segment your work into multiple PRs. Or, it would take way too long to separate the necessary sweeping changes out into smaller, manageable chunks. You may have the best intentions, but if there’s a hard deadline regarding your work item, then you’re stuck accumulating technical debt, which all collaborators must now pay interest on. This particular issue is more organizational, which leads neatly into the next section of Pull Request Etiquette…
The Pull Request Process Sucks
Maintainers, those in control of pull request policies for a given application, ultimately facilitate the PR experience. If the experience is painful, no author is going to bother creating small, easy-to-review PRs. It only takes one or two bad experiences for authors toa adjust to working around the pain, by just submitting everything all at once. This leads a vicious loop, as both sides of this coin feed into each other negatively, making things worse. Let’s take a look at some areas where maintainers encourage poor etiquette.
The Build Takes Forever
If the automated builds take 10 minutes, 20 minutes, or longer, nobody will want to stand in that line more than once. Does every automated check have to run for every review? Are there automated steps that could be deferred to later down the build pipeline, such as prior to a testing/staging deployment? Are there a section of tests (E2E, Load, etc.) that could wait for a merge into the stable branch? Explore all these options to try and cut down what essentially is a time penalty on every PR and PR re-submission.
Too Many Approval Requirements
The more eyes that have to look at a PR, in theory the better, right? Not if it leads to rubber-stamping (“four other people have to look at this and three have approved already, so my review doesn’t matter”) or other poor reviewer etiquette. From the author’s perspective, if they have to wait for a lot of people to peel away from their work to take a look, they’re not going to submit more than one PR. The more required reviewers that exist, the longer reviews will take. This is an inescapable reality I’ve seen plague an organization out time and again. There may be a desire by the maintainers for collaborators to see new changes for the sake of shared knowledge, but there are better avenues for that than blocking PR approval. Every dependency added to a pull request, complicates the process, and each new required reviewer is a dependency.
The Gatekeepers are Busy
Sometimes, personnel in a specific role (such as a senior developer position) are made to be a required reviewer for all incoming pull requests. This has the obvious benefit of letting well-trained eyes see all incoming work, but it also creates a bottleneck. This is especially the case when there is just one person in this elevated role.
A pull request that can be stretched into hours, or multiple days, due to having to wait on one person’s approval, is awful. No need to mince words here! An author cannot submit a small PR, as they have deadlines on their work. This leads to larger, singular PRs that take longer to review, and have more potential points of failure requiring re-submissions, which balloons the process even further.
It may also be the case where a collaborator leaving a comment, must make sure their comment is addressed and resolved. While they aren’t always a required reviewer, it may be policy that they become an additional gatekeeper. This makes sense, but it also means that person is taking on the responsibility of completing the review, and therefore adds another barrier to conquer. Electing yourself to this role leads to perhaps the most important piece of etiquette you can follow:
With Great Power…
If you are a Pull Request gatekeeper, whether through policy or through self-election, it is on you to not block your fellow collaborators and prevent them from getting work done in a timely manner.
If pull requests are being delayed by hours or days (both of which I’ve seen in the wild) due to gatekeeper unavailability, then that is a problem that must be addressed, full stop. Either the gatekeeping has to be delegated, or PR policy requires a review itself. Every pull request is additional work for every collaborator, that distracts from their own tasks, and roadblocking everyone for this length of time leads everyone down the road of poor pull request etiquette.
No matter how much good etiquette a PR author follows, or how solid other collaborators are at reviewing, gatekeepers can stall the entire process. It is paramount that a gatekeeper stay on top of good etiquette, to facilitate a better process for all parties involved.
As gatekeepers, there’s other good steps one can follow, in addition to the above.
Keep the Queue Clear
Pull request reviews that you are notified about, that you can potentially review, are important to address as soon as they come in. Every collaborator has to strike a balance between their own tasks, and helping others get theirs done. PR reviews being addressed quickly helps work get done, even if you are providing feedback that sends them back to re-work some portion of their code. If the queue of pull requests piles up, like other forms of poor etiquette, the cycle feeds in on itself and makes the whole process take longer.
The worst thing a pull request author has to do is start pinging collaborators individually to see that their PR is reviewed soon. That burdens an author with another PR-related task, which is another work distraction, and increases the amount of messaging and pinging around the organization (something we can all strive to have less of).
Communicate Progress on Reviews
If the pull request policy is “any two collaborators must approve a pull request”, signal that you are reviewing a pull request in some manner. Leave a comment on the PR, add a reaction emoji (👀 is my preference), or somehow tell all the other collaborators “I’m one of the two reviewers.” Rather than 10 collaborators piling into a single PR, the likelihood of too many people reviewing drops if everyone can see if others are already taking a look. If you find collaborators not saying a word until they leave a comment, or mark the PR as approved, encourage “actively reviewing” communication to help everyone out. If you end up on a call with the author to discuss aspects of the PR, add the salient points to the PR itself so that all collaborators benefit from what communication occurred.
Similarly, if you have to back out of a review due to external demands of your time, communicate this as well.
The Pull Request Process Doesn’t Have to Suck
Authors, reviewers, and gatekeepers can all strive to make the pull request process perfectly painless for all participating parties. There are easy wins that one can do to gain some ground. Other improvements, such as updating policy or making the application easier to make small changes in, can take a while to achieve but provide larger dividends. The goal of a PR, after all, isn’t to waste time or distract everyone from their own tasks. It’s to have checks in place for incoming code so that the stable branches of an application are the best that they can be. It’s to keep other collaborators at least somewhat in the loop of what everyone else is doing. It’s to improve everyone’s skills in development.
I’ve mentioned several times throughout this post that poor etiquette feeds on itself to breed a worse experience, but the reverse is also true: Good etiquette leads to a better PR experience, which in turn leads to better practices being performed. If I can submit a small PR, the build runs fast, reviewers and gatekeepers respond timely, then I’ll submit more small PRs. If authors are submitting small PRs, then I as a reviewer am encouraged to keep the queue flowing, because I know my time isn’t being wasted, and committing to a review (even elevating to gatekeeper) is not a large investment or distraction.
Collaboration is the ultimate goal, and looking out for each other is the best way to thrive in any environment.