Peer review is one specific aspect of general teamwork and collaboration. This aspect also touches on the topic of quality, reliability, project documentation and other adjacent concerns.
In future articles, we plan to expand on the broader topic of effective teamwork, and on this one we will focus on “Peer Review” as a step of the development process, focusing primarily around the creation, evaluation and resolution of Pull Requests.
Submitting Pull Requests
Below we’ll cover some of the main preparations and guidelines Lemma teams follow when submitting a new pull request:
Basic Requirements
The team should document Git conventions and best practices, such as workflows, branch naming conventions, etc.
The team may have separate processes for extraordinary situations, like submitting pull requests for hot-fixes.
- For these extraordinary cases, the team should document the process to be followed, as well as clear criteria on when this process can be used and by whom.
Pull requests must have a clearly defined scope and shouldn’t combine multiple concerns.
- For example, it’s not acceptable to submit a pull request that implements a new feature, fixes code syntax issues on a separate file and refactors a test script for another part of the application. Those are three different pull requests, no matter how small the change set involved for each.
In order to help the reviewers and for documentation purposes, the description of a pull request should detail the context in which the changes take place and the strategy and approach chosen to solve the problem. This context includes:
- Referencing the issue, ticket or User Story being implemented.
- Attaching any software design diagrams or artifacts that explain the implementation decisions and approach.
- Explicitly documenting technical decisions, considerations, risks, obstacles or anything pertinent to understanding the implementation holistically.
When to Submit a Pull Request
A pull request can be submitted anytime, even in a very early draft state, as long as the status of the pull request is indicated appropriately.
Engineers are encouraged to submit draft pull requests, as it creates an easy and natural way to share progress and for others to have transparent visibility on their work, not just in discrete increments, but continuously.
Draft pull requests also help with the implementation design phase, as it allows the team to share architecture diagrams or artifacts, have conversations in the pull request itself (if using a tool that supports that, like GitHub) and ensure technical alignment on the implementation even before a line of code is added.
When to Mark a Pull Request as “Ready for Review”
A pull request is submitted for review when the engineer believes the work is of quality that it can be shipped to production.
This does not necessarily mean an entire feature is ready. Engineers are encouraged to publish pull requests which target a feature branch. Engineers can also use other tools like feature flags to submit code that is ready to be incorporated in the production database even if it won’t be used by users yet.
Doing so helps the team review smaller bodies of work, improves the chances issues will be discovered early during a review, and ultimately improves the team’s cycle time.
Before Requesting a Pull Request Review
Before submitting a pull request, the author pulls and merges the upstream changes into their branch and manages any conflicts. They make sure to run all the automated tests and verify the software builds.
The pull request author reviews their own work, the same way they would review a pull request that someone else submitted. This doesn’t mean just reviewing the code, but also the product changes and following all of our guidelines for reviewing pull requests. If the author wouldn’t approve the pull request themselves, then it’s not ready for the broader team to review it yet.
The author ensures that the pull request contains all required considerations, context and artifacts required for understanding the work holistically. If teammates can’t start reviewing the pull request without asking several clarifying questions, then the pull request isn’t ready for review.
For complex implementations, an engineer should consider complementary collaboration prior to asking for review. For example, they could schedule an “Engineering Huddle” meeting to share the context of their work and prepare the team for the review process.
The author should also ensure that they have good commit messages. Good commit messages can provide a nice bullet-point summary of the code changes. They are the main narrative mechanism for others to understand the evolution of the work done.
Finally, the author @mentions the teams that they want to involve in the discussion and explains why. (e.g., ”/cc @DesignTeam do you think this looks good?”)
Pull Request Approval
The author is responsible for all aspects of the pull request, from submitting it to closing it.
Cycle time is a critical metric for software development teams. Allowing a pull request in a “Ready For Review” state to age directly impacts the team’s velocity and quickly adds more work to the author’s plate as the target branch changes and conflicts arise. The author is responsible for soliciting time from their peers and making the review phase a priority.
The author is responsible for merging a pull request, once the review phase is completed.
Reviewing Pull Requests
It is unrealistic to build a concrete list of requirements that encompass all of the possible situations we may encounter when reviewing pull requests. Instead, this article offers a guide for a few precise requirements while also outlining a general philosophy regarding any pull request review.
A review should not necessarily take the shape of this article where the first guidelines are always applied first, and the last guidelines are consistently applied last. This article should be considered as a whole rather than as a list of tasks to complete before allowing the contribution to be merged.
Author Responsibilities
The author of the PR proactively requests feedback and ensures that their PR is reviewed by the team in a timely manner. The author is measured on cycle time of pull requests they submit, but this is not a suggestion to rush pull request reviews. Quality and alignment are still more important.
The author takes care of addressing and resolving all feedback left from the team. Reviews may take several iterations until the PR is ready to be merged. If someone doesn’t agree with a part of the author’s implementation, it doesn’t mean the author must change it, but neither does it mean the author can ignore their teammate’s concern. They reflect and discuss until there’s a consensus. If the consensus can’t be reached, help comes from the rest of the team to work it out together.
If the author agrees with a suggestion and decides to make a change to the implementation, it’s their responsibility to then update the comment thread in the pull request and any additional relevant documentation. The author avoids commit messages like “Change suggested from PR feedback” and writes meaningful and explicit commit messages instead.
Reviewers Responsibilities
Reviewers review not only the code changes in the pull request, but also how the changes impact the project as a whole. Reviewers make sure that all implemented changes contain any required updates to the documentation of the project, not just in the main repository, but also any necessary external or tangential assets (e.g.: wikis, API docs, run-books, etc.)
Sometimes bugs occur not because of the changes added, but because of what’s missing that should have been added, e.g. not updating a configuration file for other environments.
Sometimes an implementation will effectively solve a problem, but at the expense of taking a hit to performance somewhere else or other detrimental tradeoffs. Sometimes a new feature may work great by itself but be inconsistent or redundant when taking the codebase as a whole.
Reviewers also make sure that sufficient security protections are in place, and that changes made to the codebase are consistent with the coding style and decisions on the rest of the codebase.
Reviewers do not limit their review to the submitted implementation. Instead, they ask themselves if they would implement the pull request in a different way, analyze the trade offs of possible implementations and make suggestions based on that.
Reviews don’t just evaluate the technical aspects of pull requests. Reviews also assess the quality of the user-facing change in the product. This means reviewers check out the branch locally or deploy it in isolation and perform at least basic smoke tests or exploratory testing. The objective is to experience the change as users will and be the first group to provide “user feedback”. In addition to asking “do we want this change in the product,” the reviewer is:
- looking for bugs;
- evaluating if the feature is useful and the UX is enjoyable, intuitive and consistent;
- validating error scenarios and edge cases to ensure they are all handled well within the product;
- ensuring that UI changes are accessible to people with different abilities (e.g., sufficient contrast, keyboard navigation, aria tags, etc.)
All changes that need to happen on the server or other repos (e.g. configuration files, migrations, provisioning scripts, etc) are done and reviewed before the pull request can be approved.
Reviewers are “kind to the coder, not to the code.”
It’s a reviewer’s responsibility to ensure the quality of the codebase lives up to standards of quality and to keep the feedback exchange respectful. It’s a shared responsibility of the author and the reviewer to collaborate in a productive way, demonstrating empathy and resiliency. It is never acceptable to sacrifice kindness in the name of technical optimization. The trust and synergy of the team in the long run is what drives technical standards overall.
Reviewers and authors alike are mindful of business velocity and the real costs incurred when work is sent back to be restructured. Reviewers follow our best practices for QA. Remember that the quality acceptance criteria we set for each change may be different and always a function of pragmatism and business trade offs being evaluated. For example, reviewing a standalone prototype or proof of concept should not be done with the same criteria or rubric as reviewing a new core feature to be released to a production environment.
Rejecting Fast
Every pull request is different. Before reviewing any given pull request, reviewers consider the optimal way to approach the pull request review with the objective of rejecting a change as fast as possible.
For example, a reviewer may want to do a product level review as early as possible for a pull request that includes a new UI feature. Alternatively, perhaps the author is submitting a new pull request that was previously rejected due to performance concerns and the reviewers feel it is necessary to pull the branch to benchmark it.
Cycle time, the amount of time it takes for an issue to go from start to completion is a critical metric for software development teams.
If a pull request contains items which will not be resolved within a day or two the reviewers reject the pull request or the author closes it. Rejecting or closing a pull request is the correct action and signals the actual state of the work. Rejecting a pull request is not a signal that the work is not appreciated or valued. Instead, such actions are taken to categorize work by its actual status and communicate progress transparently.
Reviewing Fast
A pull request is a gate which blocks work from progressing. Reviewers make time to review pull requests each day. This time is appropriately accounted for during team planning activities as well.
Consistency, Style, and Readability
Keeping a consistent codebase is an essential part of building sustainable software. We rely on automation to help ensure consistency and enforce a comprehensive set of linting and vetting rules through our continuous integration process.
For items that can not be easily automated, we maintain various style guides. Authors should respect these style guides and reviewers should consider them as they review pull requests.
The balance of our work falls into subjective territory. Statements like “this isn’t very readable” are not helpful given the feedback is not specific and cannot be qualified. However, that does not mean that a reviewer should outright ignore code that is hard to understand due to how that code is written.
A single “best” way to write any particular code does not exist. Pursuing such perfection is not our goal. Instead, reviewers and authors should accept that there are likely many appropriate approaches, and so long as the code is not defective and the code does not perform poorly, the contribution can move forward.
Subjective concerns should rarely be allowed to slow the team’s velocity and reduce cycle time. However, when one of the first people to look at the code is having a hard time understanding it, we can reasonably expect that block of code to become more challenging to work with over time considering the reviewer likely has a large amount of context when reviewing the pull request.
If the reviewer feels strongly that this is a problem, they raise the issue for the author’s consideration and ideally propose an alternate implementation. The reviewer is pragmatic and seeks to propel the discussion forward rather than block with obstacles.
When in doubt, reference “prior art” in the codebase, especially those around the area near the contribution.
