Pull Request
A Pattern for using PRs in a way that helps, rather than hurts integration
People have strong opinions about Pull Requests, often rooted in their (negative) experiences with certain processes. This pattern focuses on the mechanics of PRs and how they can be used to facilitate collaboration and integration — especially with remote teams, and reduce overhead when you have requirements to document reviews, release notes, etc. Maybe we need a new name for the tool?
You want a transparent mechanism for Code Reviews that facilitates Automated Checks, particularly with an all-remote or distributed team. This pattern describes an approach to facilitate code reviews that offer timely, relevant, and actionable feedback in a shared space, potentially including people who aren’t in the same room or can’t meet at the same time.
The Challenges of Remote Collaboration
Collaboration works best when people have a high degree of shared availability and bandwidth. Many teams are remote, which has benefits but also presents collaboration challenges. Tools for remote collaboration do not always support asynchronous collaboration.
The easiest way to get feedback is to ask someone to look at your code by sitting next to you or by screen sharing. Sometimes, the available person may not be the person who can give the best and most timely feedback. Limiting feedback to those who are nearby may limit insights. Team members may not be co-located.
Sometimes, someone may want to browse the code on their own before contributing feedback. Each person could download the branch locally, build it, and review it, which adds complexity to the process and limits collaboration possibilities. With the possible exception of Pair Programming, giving feedback on another change set means that the developer doing the review needs to change the context.
The Automated Checks that a Continuous Integration Build offers can provide important insights into the state of the code. You should be able to run the checks locally, but they take time. Some tools work only in a CI environment. Getting feedback in a developer workspace can be more immediate. Still, there is value in having a process to run Automated Checks and Unit Tests in an integration environment and incorporate that feedback into the review. This makes using a centralized mechanism like GitHub a possible solution.
Centralized mechanisms like GitHub can lead to asynchronous review work in batches, but they can be the best solution for remote or distributed teams. Pull Requests also allow the ability to gate merges, which can be good and bad. For an open-source project, where the contributor may be unknown and there hasn’t been an opportunity for collaboration, a gate makes sense. On a product or project team, gates can be impediments to delivery. Some more complex changes will benefit from team feedback,
You want rapid feedback to avoid delaying integration. This means getting feedback promptly and having rapid conversations. It is rare, however, that a specific person is the only one who can give feedback, and most oversights can be addressed in a subsequent change. Not all changes benefit from human reviews.
Some regulated industries require that there is some documentation of reviews, and you want to minimize the overhead of that documentation process. Unrelated to any compliance requirements, having data about the review process (timing, changes, etc.) might be useful for tuning the process and identifying process issues that you can address in a Retrospective.
You want to balance the speed, quality, and relevance of feedback.
Leverage Tools in an Adaptive Way
Establish a Pull Request Process that encourages collaboration and rapid feedback. Favor interactive reviews in a time frame that works for the team. By default, avoid blocking Pull Requests for all but the most significant changes; trust team members to address issues or to reach out for clarification.
The essential elements of a Pull Request are:
A way to announce that code is “ready to merge into the Main Line.”
A way to request feedback (synchronously or asynchronously) in the form of comments or proposed changes
A way to share responses to feedback in the form of comments or code changes.
A guarantee that automated builds and checks can run on the code before a change is integrated.
The following things are not essential elements of a Pull Request:
Integration Gated by approval: While approval can be part of the process, a sense of shared responsibility makes for a better delivery pipeline.
Long asynchronous comment threads: Asynchrony can be useful when there is a Time Zone difference between teams (for example, sharing end-of-day work for another team’s feedback might be useful), but the goal should be to integrate as quickly as possible.
A Pull Request is a form Code Review that is done in a shared environment. This enables:
Easier collaboration across locations.
Automated checks to be run, and feedback from those checks shared in a common place.
A place for team members to share feedback and propose specific changes.
While Pull Requests are often asynchronous, they need not be; feedback can be collaborative, asynchronous, or a combination, depending on the team’s agreements. Even with interactive feedback, a brief delay for checks to run and for team members to read the code asynchronously can be beneficial.
A typical flow involving a Pull Request can include
Push changes from a workspace to a Task Branch.
Wait for Automated Checks to Run (and correct any issues).
Share, in a Slack channel, that the code is waiting for feedback.
Get the Feedback, asking questions
Addressing the feedback.
Merging after the feedback cycle is complete.
The duration of this cycle can vary by team but should support whatever integration cadence the team is aiming for
A Pull Request can support a Code Review that gives prompt, relevant, and actionable feedback.
Prompt by allowing feedback to be delivered in a timely manner, synchronously when it makes sense. An interactive review, perhaps facilitated by screen sharing, can yield the most prompt, highest-quality feedback for nontrivial comments.
Relevant, by enabling those who are not immediately available to consider code changes and by making it easy to defer the more mechanical changes to Automated Checks.
Actionable, by providing a platform for clarifying changes quickly and enabling suggestions.
A Pull Request will be effective with certain practices in place:
Working agreements about collaboration:
How quickly review should happen
Times when people will be available for collaborative reviews.
Interactive Reviews:
After an initial review period, review changes together, ending with an action plan of what changes the developer will make before merging.
Conditional Approvals for asynchronous comments.
When comments happen asynchronously, trust that the developer will address the concerns or reach out for clarification. Save “Request Changes” for significant errors. And ask to meet.
Limit the use of gates to things like:
Failed Tests
Failed Automated Checks that are known to indicate high-severity issues,
Keeping the change sets small to minimize the other factors that slow down Code Review.
Cautions
Be mindful of pull request processes that take too long. Consider gathering metrics, such as “time from open to close”, “time to first feedback,” and potentially “number of comments” to present at a retrospective.
A Pull Request will be a good experience if all the criteria that make for a good Code Review are available, such as:
A major problem with Pull Requests is a lag in starting (and finishing) reviews. This can be due to reviews not being a priority. Having the right Team Focus can mitigate this
Feature Partners can help ensure that the reviews focus on the right things and are quick, and that reviewers have the correct context.
Automated Checks enable the humans on the process to higher level issues, leading syntax, static analysis, etc tools to catch the things they are best at.
Small Development Tasks can ensure that the code being reviewed is an appropriate size to be reviewed quickly and that the code can be merged independent of other changes (atomicity).
While getting feedback is often helpful, and synchronous conversations can mitigate many risks related to delays, keep the review process aligned with risk level, and be wary of falling into a habit of synchronous feedback. Some changes might not need a formal review, and sometimes, asynchronous feedback makes sense. However , long comment exchanges can be a sign of deeper collaboration issues. Continually evaluate the process.
Notes
Consider making PR gates contingent on risk, perhaps using a tool like Shepherdly , which can assign a risk score to a change