An Opinionated Guide to Pull Requests (PRs)

Everything in this chapter is predicated on the notion that getting our code into production is the single most important thing that we do as product engineers. Our code creates value for the customer. And, releasing that code incrementally—using feature flags—creates a fly-wheel of feedback that leads to more inclusive products; and, creates the psychological safety that the EPD organization needs in order to iterate effectively. But, this value is only ever realized after our code reaches production and becomes available for consumable.

Deploying code is, therefore, an act of love and generosity; both for our internal team and for the external customers. The entire process preceding deployment must be optimized in order to maximize the value that we deliver—as engineers—to the people that we're serving.

There are many steps that need to be completed when taking a project from conception to deployment. If any one of these steps creates a bottleneck, the entire process is delayed; and, as a result, the customers suffer.

More often than not, the slowest and most unpredictable step in this process is the pull request review. It's not uncommon for a PR to remain open for days; and, every now and then, I hear from an engineer who waited weeks for their code to be reviewed and approved.

Frankly, this is unacceptable. If a PR remains open for more than 15 minutes, your process has failed you. And, more to the point, your process has failed the customer.

I hope that this assertion tickled you. In fact, I hope that you see a 15 minute PR window as being entirely absurd; because, if you do, it means that there's so much potential here for you and your team to unlock.

Note: When I refer to the "team" in this chapter, I am referring to an engineer's vertically integrated team within the larger engineering organization. This is the small group of individuals who typically collaborate on the same areas of the application.

Of course, there will always be exceptions to the rule—not every pull request can (or should) be reviewed in 15 minutes. And, to be clear, a lot of changes in your process will need to occur in order to make this time-frame realistic. But, I do believe that this can become your baseline. And, I do believe that a streamlined PR review process is essential for a high performing product engineering team.

#WIP vs. Ready

There is nothing inherently meaningful about a pull request—it is just a place to put code. Sometimes, a PR is created for personal review: a place in which to see your own code in a different context. And, sometimes, a PR is created for external review: a place in which to share your code with your fellow engineers.

In this chapter, I'm specifically talking about pull requests that are ready to be deployed (at least, as far as the authoring developer is concerned). I am not intending any of this conversation to be applied to WIP PRs.

WIP stands for "Work in Progress". These are PRs that contain raw, unfinished thoughts. A WIP PR is intended to be used in a brainstorming session and provide a jumping-off point for architectural or technique-related discussions.

There is no time frame expectation around a WIP PR. Nor is there any expectation that a WIP PR will persist; or, even resemble any future PR that is prepared for deployment. WIP PRs are a transient artifact.

If your team does not inherently discern between these two types of PRs, I suspect that you have a communication of intent problem. Or, maybe a trust problem. Or, a division of responsibilities problem. Or, some other type of breakdown in team organization.

Fixing this is simple. Just add [WIP] to your pull request title if your intent is to prime the reviewer for a deep, critical feedback session. And, omit [WIP] if your intention is to seek approval for ready to be deployed code. And then, communicate that this is now the expectation for the team.

This will force the code authors to think—with intention—about how they want their code to be reviewed. And, it will tell the reviewers what level of feedback is being requested. This is what good communication does. And, it lays the foundation for working with enhanced clarity and improved efficiency.

#Define Your "15 Minutes"

I can testify that my team operated in the 15 minute window for years. It wasn't every PR; but, it was most of them; and, the experience was spectacular!

But, I don't want you to get hung-up on the 15. If it feels more comfortable, make it 30 minutes; or, 60 minutes. It's not so much the amount of time that matters, it's the fact that it sets an expectation within the team.

Most teams that I talk to have no expectations at all about the PR review process. I hear managers say things like:

"Our developers probably review PRs once or twice a day."

Or, from an individual engineer:

"I'm heads-down most of the morning; so, I usually do my PRs in the afternoon."

This loosey-goosey process leaves no room for accountability. And, without accountability, engineers don't know what to expect of each other. And, they have no idea what others expect of them. And so, we need to be explicit.

It's important that this time not be a reflection of what is, but, rather, an assertion of what is acceptable. To frame this thought, it helps to imagine that each pull request is a blocking operation. Meaning that after sharing a PR, the author must sit there and wait for it to be approved before they can move on with their work. As a team, you need to decide how long you're willing to let your teammates sit idle, waiting to be unblocked.

Of course, most people won't sit idle, waiting around for approval. But, by defining a bounded window and setting the expectation that such a window will be honored by the team, it creates a predictable decision-making context in which people have an opportunity to manage their time more effectively.

#Everyone Is Responsible All the Time

In order for this process to work, everyone needs to be responsible all the time (during business hours). I know that some teams like to establish an on-call rotation where the on-call engineer is the one engineer responsible for PR reviews. But, this is no longer a reasonable way to organize the team when your primary goal is shipping product and serving customers.

Shipping product needs to become an all hands on deck process. Which means, when a pull request is ready for review, everyone relevant needs to know about it; and, they need to know about it now.

As such, you cannot rely on hope to drive this process. Meaning, you can't assign a PR to your teammate (in GitHub or Bitbucket) and then hope that they see this assignment. And, you certainly can't hope that they check their email; or, hope that they have some chat-bot integration that alerts them to said assignment.

Instead, you have to be direct in your communication. Each team's communication strategies are going to be different; but, to make this concrete, here's an approach that worked well for my team: we would go into our team's private Slack channel and then post an @here message with information about the size of the PR and what the PR sought to accomplish.

An example:

@here 2-line PR that adds a few properties to the database query that I will need on the front-end — {link to PR}

Another example:

@here Small PR that adds analytics tracking calls to the front-end button / link interactions — {link to PR}

Another example:

@here Medium PR that adds the CRUD (Create, Read, Update, Delete) SQL queries for the new widgets entity model — {link to PR}

Another example:

@here Large PR - apologies ahead of time, I couldn't figure out how to make this PR smaller. I needed to move some code around, so there are a lot of files being touched; but, not much of the functionality is actually changing. — {link to PR}

At this point, me and my teammates would see this message, figure out if the description matched our skill-set, and then—if possible—"claim" the PR review by adding an "eyes" Reacji (👀) to the Slack message. This emoji indicated that the PR was being handled by one of us; and, allowed the rest of the team to return to their work.

Then, anywhere from 30 seconds later (for a one line review) to 15 minutes later (for a large review), the reviewer would add a "thumbs up" Reacji (👍) and @-mention the PR author, explicitly letting them know that the review is complete; or that further revisions are being requested.

This explicit two-way communication minimizes the amount of engineering down time. Which, in turn, maximizes the amount of code getting shipped to production.

By bringing a formal structure to the PR review process, we remove a lot of emotional baggage. That said, using an @here message to announce a PR can be a large emotional hurdle for many engineers. Because of the past traumas that we've endured while working on under-resourced, over-scoped, and over-due projects, the very notion of taking up other people's time can be quite triggering.

You have to flip the story. You're not taking up people's time—you're giving them an opportunity to help ship code and service the customer. Conversely, when other people need a review, they're not taking up your time, they're giving you an opportunity to do the same.

Until this process feels natural, it can be helpful to act overly enthusiastic in your response (ie, fake it 'till you make it):

Changes 👏 look 👏 great 🚀 ship 🚀 ship 🚀 ship!!!

The more you emphasize the excitement around each change, the more you'll begin to understand that you are an important part of the change; and, by extension, an important participant in the servicing of the customer.

And remember, this responsibility isn't on any one individual, it's on the team. Even if 5 PRs are posted, you may only be fast enough to claim 1 or 2 of them. So, whatever insanity you're currently picturing in your head, take that measurement and divide it by the number of people on your team.

Aside: It can be tempting to draw parallels between this PR workflow and alert fatigue. But, you have to remember that alert fatigue takes place when there's a steady flow of negative outcomes. Alert fatigue does not apply to positive outcomes. Pavlov's dog didn't develop alert fatigue from the bell, it learned to become excited by the sound of it.

#Consider Using the Buddy System

If you truly believe that including your whole team is a step too far, consider starting with a smaller group of like-minded individuals. Seek out the teammates who are dissatisfied with the status quo and form a voluntary "PR Buddy" system. Then, instead of using @here when requesting a PR review, @-mention your "buddies".

Once you start working small and building incrementally using feature flags, it doesn't take much to create a high-throughput PR review process. Even having a single PR review buddy is enough to radically transform your workflow. In fact, starting with a single buddy might be the most efficient option since your shared enthusiasm will be high and your expectations will be very clear.

It's always easier to lead by example. This is especially true when the desired cultural change faces a high emotional barrier. When other people on the team see how much more productive your experimental buddy system has become, they will be more inclined to join the revolution.

#Work Small and Build Incrementally

As we discussed previously (see Life-Cycle of a Feature Flag), feature flags revolutionize product development by—in part—enabling small incremental changes, each of which can be deployed safely to production. When considering this opinionated pull request review process, you must remember to think about it in the context of feature flags, not in the context of a traditional waterfall methodology.

When your teammate uses @here to message the channel, the PR they're posting doesn't represent 3 months of work; it doesn't contain 137 files; and, it doesn't create 6 new database tables. When using feature flags, each PR should represent a relatively small and highly focused amount of work.

Focused work is always easier to review because it minimizes the scope of change. And, as you decrease the scope of change, you increase the number of people that are eligible to review the PR. As an extreme example, if you isolate a small spelling mistake, everyone from a front-end novice up to a database admin should be comfortable jumping in and offering approval. This significantly lightens the load across the team.

Of course, not all focused changes are simple changes. But, even when a focused change is highly complex, working small still makes the process easier. When you isolate complex changes, you decrease the number of people that have to review the changes. So, instead of getting one person to look at the HTML and one person to look at the SQL and one person to look at the business logic, all you have to do is get the one person you need for the one isolated change.

When you start taking large projects and breaking them up in to small tickets, one pattern that you discover is that only a small portion of the work requires high cognitive load. The rest of the work is much less about innovative deep thinking and is much more about color-by-numbers. As such, a good number of your tickets will lean towards the simple end of the spectrum; and, can be reviewed by a wider audience.

Small changes are also less disruptive; and, not just for the reasons above. You have to remember that when a team commits to working small, it's not just you, it's everyone. So, when you have a small PR that needs review, you don't have to feel guilty about interrupting other people. After all, they're not working on massive projects that take 3 months, contain 137 files, and create 6 new database tables—they're also working on relatively small and highly focused tickets. As such, when they take a few moments to review your PR, it won't derail their work in any meaningful way.

It is only through the consistent use of small PRs that a team is able to set—and meet—expectations around the review process. If your team doesn't learn to work small, the PR review process will always represent a fundamental limitation of your team's ability to get work done.

Of course, working small isn't really possible until you start using feature flags.

#Pull Request Size Must Be Announced

When sharing a PR with the team, it's important to provide the size of the PR in the announcement. This doesn't have to be a high-fidelity value—a rough t-shirt size (small, medium, large, etc) is good enough to provide several important benefits.

First, having to articulate the size of a PR forces its author to be more cognizant of how they are organizing their work. A huge value-add of feature flags is the ability to work small; but, working small doesn't happen by accident—it happens through consistent effort. The more we can remind ourselves and others that size matters, the better the review process becomes over time.

Second, it gives would-be reviewers a rough sense of much time the PR review is going to take. This, in turn, helps each teammate decide whether or not they'd like to participate. Ideally, everyone on the team is scrambling to help ship code; but, sometimes, people do need to be heads-down. And while it might be OK to switch context for a small PR, your teammate might not be willing to do so, at this time, for a large PR.

Lastly, it creates a healthy sense of shame about posting large PRs. Normally, I would say that we never want to shame anyone on our team; and, I'm not suggesting that we explicitly shame anyone. But, change requires tension; and, healthy tension leads to healthy change.

Large PRs are toxic for your team. A large PR might make your life easier; but, it makes everyone else's life harder. If we set an expectation—as a team—that most PRs should be small, it's healthy for people to feel a little embarrassed when their PR is large (especially if they're consistently creating large PRs). This social pressure leads to smaller PRs. And, smaller PRs are the life-blood of an efficient engineering team.

#Isolate Large, Cross-Cutting Changes

Even when trying to work small and build incrementally, it's possible for the scope of a ticket to grow beyond its natural boundaries. This is especially true when an engineer has to make large, cross-cutting changes in order to unblock smaller, more focused changes.

Take, for example, adding a new vendor library in order to execute on a new feature. Since the vendor library doesn't do anything in and of itself, it's not uncommon for an engineer to both add the new library and build the new feature in the same PR.

But, this is a mistake. The vendor library represents a large, cross-cutting concern (even if it's not being used yet within the application). Such changes need to be isolated within their own PR so that the scope of each PR remains narrow and easy to review.

Which—to be clear—means that each new vendor library must be added and deployed to production before it's even being consumed in the application.

This may seem like unnecessary process; but, it actually showcases the value-add of working small. A vendor library contains code that was written outside of your organization; which means that there are both licensing and security considerations that have to be reviewed and approved by the Legal team and the Security team, respectively. By isolating vendor code changes within their own PR, it allows you to initiate the Legal and Security reviews early; and then, go build the consuming feature in parallel.

It's quite likely that the Legal and Security reviews won't be completed within the team's 15 minute review window—this is just a fact of life. But, by working small and building incrementally, you can parallelize the blocking work with your feature work and keep the momentum moving in the right direction.

Generally speaking, all large, cross-cutting changes should be isolated, reviewed, and deployed independently. When working with vendor libraries, these changes are easy to see ahead of time because the work is so clearly delineated. But, sometimes, a large, cross-cutting change emerges from the current work in an unexpected way.

In such cases—after the cross-cutting change reveals itself—I recommend going back to master and creating a new feature branch where you can implement and deploy the cross-cutting changes in isolation. Then, once deployed, you can go back to your original feature branch and git rebase on master, pulling-in the cross-cutting changes before continuing on with your smaller, more focused changes.

#Completed Code is the Highest Priority

If the primary goal of a product team is to serve the customer; and, if the primary means of doing so—as an engineer—is to ship code; and, if the only thing blocking code from reaching production is a pull request; then, it stands to reason that reviewing PRs is the most important work that anyone can be doing.

You might think that the code you're working on is more important. You might think that you're too busy right now to stop and review a PR. But, the reality is, your code is nothing but theoretical. Until the code is done and ready to ship, your code is just an idea.

At any moment, you could be hit by a bus. Or, win the lottery and quit. Or, the company might pivot and your project is no longer relevant. In my career, I've seen many large "important" projects come and go without ever seeing the light of day—I've even worked on several of them.

You just can't know what will happen. The only thing you can be sure of is that when someone has a PR to review, their completed code is closer to adding value to the product—and to the customer—when compared to your incomplete code.

If you want your code to become a high priority, figure out how to break it up into small, meaningful changes, put them behind a feature flag, and then ship them incrementally.

#Reviewers Aren't Responsible for Bugs

When you write code, you are the only one responsible for ensuring the success of that code. It is not the job of the QA team to catch bugs. It is not the job the PR reviewer to catch bugs. It is the job of the code author, and no one else.

Aside: Obviously, the QA team's literal job is to catch bugs. But, in this conversation, I'm speaking from the engineer's perspective, not the organization's.

When you use the waterfall methodology, and you work in isolation for weeks or months on end, your standard of working consists of sharing PRs that compose all manner of code, from low-level business logic up to superficial CSS changes. And, in a world like that, deploying code is so terrifying that having another engineer look at your code is a meaningful way to achieve some peace of mind.

With feature flags—as the scope of each ticket shrinks and the code becomes more focused and the size of each PR is minimized—the role of a reviewer changes. Having other people look at your code becomes a nice to have—an opportunity for someone to randomly spot a bug with fresh eyes or spread tribal knowledge that may not be readily available to all engineers.

If a reviewer ever needs to pull-down the code in a PR and run it locally, this is a huge red flag. It is a sign that the PR is either too big, too confusing, or contains too many novel ideas. Or, that the reviewer incorrectly assumed that it's their job to ensure the correctness of the code (which it is not).

This is not to say that collaborating on work is a "bad thing"—collaboration is an important part of team work. But, the PR review process is not the time or place for collaboration to happen. That's waterfall thinking. This type of collaboration either needs to be done earlier in a WIP PR (see above); or, in a planning phase that is executed before a PR is generated.

And, if the goal is to collaborate around a user experience, the engineer should deploy the code to production where it can be safely consumed behind a feature flag.

#Asking For a Rubber Stamp is OK

If we're being honest, once we start using feature flags to incrementally build a product, a lot of our PRs only exist to check a box in our company's security mandate. When all code must be approved by someone other than the authoring engineer, we have to create a PR for changes that don't merit review.

In such cases, it is completely legitimate to say as much when requesting a review. This is especially true when a large formatting change is being applied:

@here Large PR - I had to fix the indentation in dozens of files (someone's IDE was using spaces instead of tabs). There's no functional change here, please rubber stamp this PR. — {link to PR}

A high-functioning team works by clearly setting expectations and then trusting each other to meet those expectations. If someone sets the expectation of a rubber stamp, a healthy team should trust that they know what they're doing.

And, if you can't trust people, that's a problem that needs to be solved outside of the PR review process.

#The master Branch Must Mirror Production

Feature flags are wonderful; but, they do increase the complexity of the code. The multi-environment, dynamic runtime behavior that makes feature flags so helpful is also what makes control flow execution harder to understand. This is why we make it a priority to remove feature flags from the application once they are no longer needed.

In this respect, we must commit to having the master branch mirror the production code. When an incident occurs, our product engineers must be confident that the code they see in the repository is the same code that's running in production. If there's lag time between merging into master and deploying to production, it creates uncertainty; which makes debugging and mitigating an incident more challenging.

Of course, by the very nature of build steps and rolling deployments, there will always be some lag time before the production environment fully mirrors the master code. But, this lag time must be measured in minutes. If the lag time is ever intentionally measured in hours or days or weeks, it's a problem. And, you need to work with the EPD organization to figure out what's going wrong.

#Your git Branch Name is Irrelevant

When using the waterfall methodology—in which a feature is developed in isolation for weeks or months—having a meaningful branch name can be helpful. With the waterfall methodology, the longevity of the branch greatly increases the chance of interruption; and, the vastness of the branch means that such an interruption is cognitively expensive. In such a scenario, a good naming convention can help to reduce this cognitive load and facilitate better collaboration.

But, when using feature flags, the name of your git branch no longer matters. Feature flags allow us to take large projects and deconstruct them into many smaller tickets, each of which is independently deployed. In this workflow, a git branch corresponds to a single ticket. Which means that it may only exist for an hour or two before it's merged into master and deployed.

When your branch only exists for an hour, the name of the branch doesn't add value. Of course, you can name your branch whatever you want. If you want to name your branch something like, product/checkout/new-flow, go for it. But, any attempt to mandate such a naming convention at the team level serves only to create yet another point of friction for your engineers.

Some teams mandate that a ticket number be included in the branch name, such as, product/AUTH-123 (where AUTH-123 is the ticket number). This is an unnecessary requirement. If you need to associate a git commit with a ticket, simply include the ticket number in the commit message itself. The intention of the commit message is to describe the change being applied to the application; which makes the commit message a very intuitive place to reference an external ticketing system.

#This All Hinges on Feature Flags

As you read this chapter, I've no doubt that you had all manner of immediate push-back. And, perhaps that's because you're reading this from your current perspective with your current team dynamics and your current operating procedures. I'm trying to describe a PR review process which is only possible through a fundamental shift in perspective.

This shift in perspective cannot—and will not—happen until you start using feature flags. Feature flags allow you to work small and ship iteratively. Which, in turn, allows your PRs to shrink. Which, in turn, transforms the emotional weight of each PR. Which, in turn, changes how they slot into your daily workflow.

If your approach to product engineering doesn't become feature flags all the way down, your PR review process will continue to be a limiting factor in your team's throughput. And, the lack of throughput will ultimately limit your ability to serve the customer.

#Trunk-Based vs. Feature-Based Development

In the PR review workflow that I've outlined, you're creating feature branches. But, those feature branches are intended to be short lived; and, are merged into the master branch and deployed to production soon thereafter. So, you might be wondering if this approach to work is considered Trunk-Based or Feature-Based.

The good news is, it doesn't matter what you call it. Once you make feature flags a core tenet of your development philosophy, the differentiation between branching strategies becomes irrelevant. Many of the branches you create will only live for an hour or two; but, some will live for days. As long as your team's strategy revolves around working small and shipping often, you can call it whatever you like.

Personally, I just call it, "The Way".

Have questions? Let's discuss this chapter: https://bennadel.com/go/4561