PRs: Shift Left, Please (Part One)
Individuals and Interactions Matter When Reviewing Code
The pull request (PR) process seems an ideal way to ensure that our systems retain high code quality. Using pull requests can “promote cooperation, participation, and collaboration” within teams using it. Yet the PR process is the least collaborative out of all the review options available to you, and probably the poorest choice as a result.
Benefits of the PR Process
To initiate a review of work I just completed (alone), I create a PR through GitHub (or another site like Bitbucket or Gitlab), which notifies other developers that my work is available to review. On their own time, my teammates can easily review the changes I made. We can interact asynchronously: reviewers can comment on the changeset as a whole or on specific lines of code I changed, and I can respond to their comments. Once I make requested fixes (or decide not to), the reviewers can accept my work by merging the changes into the trunk (mainline) branch.
The PR process is appealing to developers and onlookers as well since:
- It provides a formal stamp signoff with traceability if anything should go amiss.
- It can provide confidence in software by gating quality problems.
- The GitHub interface makes the process appear simple.
- It appears to minimize effort and streamline the entire review process from end to end.
- It avoids the need for people to meet face-to-face.
Done well, the PR process can provide value.
Chances are good, however, that you’ve already noted ample challenges with the PR process. I’ll make my own bold claim: the PR process is too costly for the minimal value it provides and the overconfidence it creates.
If you’re already convinced that PRs are the only way to go and you’ve decided to stop reading before I make my case, at least first jump over to read “Stop! You’ve been using pull requests all wrong.” That article contains good advice for succeeding with PRs, including the notion that PRs should be tiny (“could be 20 lines”) and that you might instead consider trunk-based development.
If you’re still reading, however, here are my more-specific claims. The PR process:
- is done poorly in most shops
- is not agile
- can erode your culture
- is inherently ineffective
- is inherently inefficient
PRs Are Done Poorly in Most Shops
I’m obviously not the first person to make the first claim that PRs are generally done poorly. Here are a few articles that point out significant problems with the PR process:
You’ll find numerous similar articles, most of which also provide recommendations for improvement. I’ll add my perspective in brief: I’ve almost never seen highly effective results from the PR process in the numerous teams I’ve been part of.
As I hinted in my overview of the PR process, it’s absolutely possible to do PRs well and obtain some benefit from doing so. The reality of outcomes is a different matter; I believe that’s because the process itself is inherently ineffective and inefficient due to its emphasis on minimizing human interaction.
PRs Are Not Agile
Your first reaction to the notion that PRs aren’t agile might be, “So what?” But the implication — that PRs run counter to agile values — can provide insights as to why they’re not effective.
Surveying the outcome of two decades of agile dominance, you might conclude agile means pretty much anything that anyone wants it to mean. But the Agile Manifesto gives us its real definition in the form of four core values, each expressed as a scale from more agile to less agile.
The first agile value scales from individuals and interactions (left) to processes and tools (right). We’ll always need processes and tools in software development, but to be truly agile is to increase the amount of collaborative work we do and to depend less on processes and tools to ensure we succeed. A couple of agile principles from the manifesto explicitly support this interpretation:
- Business people and developers must work together daily throughout the project.
- The most efficient and effective method of conveying information to and within a development team is face-to-face conversation.
Mapping the possible ways of reviewing code on the same agile scale — from a focus on individuals and interactions to a focus on processes and tools — might look like this:
📝 Note: definitions and names for the more-established review practices — the ones inbetween pairing and PR — are imprecise.
- Mob programming. Mob programming, also known as ensemble programming, provides an active form of review where the entire team interacts continuously and must agree on quality before moving on. Mob programming is leftmost on the scale, with a maximal focus on individuals and interactions and a minimization of tools and processes to manage the experience.
- Pairing. Pair programming, where subsets of the team interact as collaborating pairs, is similarly active. Two heads can still produce solutions of questionable quality. Swapping out members in a pair prior to finishing a task provides a third set of eyes that can help spot and rectify problems before it’s too late.
- Informal reviews. An informal review process incorporates post facto interactive sessions (that is, occurring after the code was created) that involve questions and discussions around the code; no artifacts are typically created.
- Walkthroughs/inspections. Formal walkthroughs or inspections are more rigorous, disciplined processes that follow a specific structure, perhaps based on IEEE standard 1028. Fagan inspections require reviewers to prepare a review document prior to a central meeting between the code producer and reviewers (the more the merrier). This preparation helps streamline an otherwise costly joint meeting — and the meeting itself is further structured in a way that minimizes the amount of interaction needed between attendees.
- PRs. Pull requests can be as disciplined — and costly — as reviewers or teams choose them to be. However, they’re the most devoid of actual human interaction since they promote written communication in the form of comments made via a web app.
My review process scale doesn’t necessarily correspond to the traditional meaning of shift left, which is “to strive for doing things earlier in the development cycle.” Here, a shift to the left represents an increase in interaction.
If you shift left far enough, to pair programming or mob programming, you are doing a true shift left: the review process occurs earlier, moving from after the work is deemed complete to while the work is being developed.
Being rightmost on the review process scale, the PR process is the least agile of commonly employed review mechanisms. You repeat, “So what?”
In Part 2, I’ll explore the implications of de-emphasizing human interaction, and explain how the PR process, as a result, is ineffective, inefficient, as well as how it can damage your team. You’ll want to consider shifting left toward a review practice that increases individuals and interactions.
Books
Books by Jeff Langr available from The Pragmatic Bookshelf:
You can save 35 percent with promo code 6f710ef966 now through April 30, 2022. Promo codes are not valid on prior purchases.
Articles
If you enjoyed this article, you might also enjoy these articles from the archives of PragPub magazine:
