Pull Requests — checklists, metrics and best practices,
a definitive guide
Many people ask what's a Pull Request. Being straightforward, a Pull Request is the name of a GitHub feature that aims to easy the code review process by improving its experience. Before this feature, software engineers and developers used to send patches of code by email. Patches are files containing code changes (both removed and added code) condensed in a format that developers can apply them on a codebase.
Linux development used to share patch files via email. As new tools came out, the process of development modernized itself. Some tools call it Merge Request, or Change Request. In practice, the same thing: an evolution of email threads with patches attached to it. And Linux now uses Pull Requests to develop the operational system.
The Pull Request feature is hugely influenced by open-source philosophy. That’s why it is used by modern companies to promote collaboration and a sense of community among peers.
Roles involved in a Pull Request
As it is a kind of peer review, the primary people involved in a Pull Request are Software Engineers or Developers. However, other roles can benefit from practice.
Tech Leads should review pull requests in a less intrusive or blocking approach. By overseeing the team’s work, tech leads can understand technical deficits, or spot technical misguidance. It’s their work to keep an eye on the produced code and make sure it meets conventions and standards.
Team Leads can look at the comments for engagement and lack of specific knowledge. Besides, comments reveal how people communicate. A non-violent language and a mentoring mindset (the one that stands for questioning and teaching instead of pointing fingers) can reflect in a more harmonious work environment.
As the hierarchy counts, sometimes engineers tend to avoid discussions or even feel uncomfortable when leaders actively take part in the review process. That’s why Engineering Managers or CTOs are not encouraged to participate, although overseeing can bring lots of benefits, including the mitigation of risks.
Non-technical roles are welcomed in the pull requests as well. UI or UX Designers, for instance, play an essential part in reducing the feedback cycle. As they can see screenshots, run the code (even test some variations if they code) in their machine, they can aggregate a lot of the process.
Pull Request best practices
The Pull Request itself has lots of best practices. They aren’t restricted to the tool, like GitHub or GitLab, but they extend to the individual, team, and technology. Plus, they can happen before, during, or after the creation of a pull request.
The best practices that come before opening a pull request intend to lessen the communication noise, making it more straightforward to review and accelerate the feedback loop. In this category, linters are one of the most known and recommended.
They can analyze static code, and look for optimization, code smells, security issues, and many other checks. Depending on the language or framework, there may be a linter for specific tests. Here a brief list of some existing engines.
Softwares used during the development process can have their best practices too. It’s the case of git.
During the review process, it’s crucial to establish a non-violent communication, as it promotes a healthy place for collaboration of all peers.
Pull Request Checklist
A Pull Request Checklist is never complete. The following topics are an excellent start for those that had nothing. However, it’s crucial for your team to adapt accordingly to its needs.
It’s recommended to review the items periodically, so the team can remove deprecated ones, and polish those that remain in the list.
This document organizes the checklist in three categories to better understand when to perform each action.
Following this checklist seems unpractical for the long run since every reviewer should adopt it for each pull request examined. It’s excellent for beginners, though, that are not confident or aware of what to look for in code reviews.
Before opening the Pull Request
Software Engineers and Developers should perform the items listed below every time they open a new pull request. The checklist aims to reduce the noise among the author and their reviewers. Besides, it also increases the overall quality of the codebase and avoids new bugs.
For more details on the items, you can read more about[[what to check before assigning a Pull Request]](/blog/10-items-to-check-before-assigning-a-pull-request-to-someone-plus-a-bonus).
- ☑️ Name the branch accordingly to the team’s convention
- ☑️ Explicit whether the Pull Request is a draft (or WIP)
- ☑️ Go through the changed code looking for typos, TODOs, commented LOCs, debugging pieces of code, trailing whitespaces, etc.
- ☑️ Make sure objects, structures, and variables have semantic names
- ☑️ Run the proper linters, and fix the pointed issues that make sense
- ☑️ Squad, edit or reorder the commits to make it look like a story (or a task list)
- ☑️ Cherry-pick non-related commits to a specific branch
- ☑️ Optimize SQL queries to fetch only relevant table columns, avoiding n+1, or fetching unused relations
- ☑️ Rebase your branch onto master and upstream branch
- ☑️ Enrich the description with detailed descriptions, explaining the motivation, and the solution. Add media like screenshots, gifs, or even small videos. Tag it following the team’s convention.
Additionally, right after opening the pull request, comment inline any interesting fact or piece of code that you feed the urge to share. For example, you may point out a new API method that got you impressed with or a programming language feature that you’ve just learned.
Reviewing a Pull Request
Take time to review a pull request properly. Reviewing code is not just about pointing fingers. It’s also an opportunity to learn from others.
- ☑️ Does the code work? Check whether function and logic are correct.
- ☑️ Are functions, methods, and variables adequately named?
- ☑️ Are names semantic and meaningful for your business?
- ☑️ Is it well tested?
- ☑️ Are there unit tests, and they have good quality? Do they test the functionality?
- ☑️ Do tests reach acceptable coverage for your project?
- ☑️ Is the code clear and easy to understand?
- ☑️ Does it meet the team’s guidelines and style guides?
- ☑️ Is there any duplicated code?
- ☑️ Is there any commented or unnecessary code?
- ☑️ Does the code take the most out of frameworks and language? Is there any custom implementation of native or already-existing functions?
- ☑️ Are there code smells, grey areas, or bug-prone?
- ☑️ Is documentation on functions, methods, classes, contexts, and behaviors adequate?
- ☑️ Is the code as modular as possible?
- ☑️ Are the critical spots adequately logged?
- ☑️ Does the code consider failures? Is it just considering the happy path?
- ☑️ Are there better or simpler solutions?
- ☑️ Is there any performance issue?
- ☑️ Are input data sanitized?
- ☑️ Is there any SQL Injection point?
- ☑️ Is sensitive information being encoded or encrypted?
Receiving reviews in a Pull Request
Pull Request benefits from collaboration. So, it’s crucial to detail the change to the team. With a better idea of the business and development contexts, they can understand and better review your code.
If it’s a work in progress, make sure to make it explicit. You can use checkboxes or bullet lists to display the tasks that need to be done. Then, follow the following guidelines:
- ☑️ Reply to the given feedback with your thoughts on the pointed issue. It can be an emoji or a single sentence.
- ☑️ Use and abuse some pull request features, like sharing specific code lines, diffs, and links to make your point
- ☑️ When commenting, link the “commit sha” to illustrate how and where is the related change.
Pull Request Metrics
Pull Request metrics are actionable indicators used for managing software engineering teams. They differ from Agile Metrics as they don’t reflect the whole process, but fundamental elements of software development. That’s why they’re called Software Engineering Metrics.
Lead Time also called Cycle Time, although they’re not the same thing, is the most known engineering metric. Agile coaches and Scrum Masters usually measure User Stories Lead Time (the time a card took to go from “Backlog” to “Done”).
Engineering Managers have been using Lead Time to understand how much time engineers spend on code review. Pull Request Lead Time, Time to Merge, Time from First Commit to Deploy, and many other metrics are[[metrics extracted from pull requests]](/blog/50-shades-of-lead-time-measuring-each-part-of-the-development-process).
Another crucial metric is the Throughput, which stands for the number of Pull Requests merged within a given period, for instance, in a week, month, or quarter. It provides a sense of volume of produced work, whereas Lead Time is more about how fast the team produces it.
By looking at both metrics simultaneously, engineering managers can understand the team’s pace, and therefore make forecasting projections or make other decisions based on it.
Start collecting Pull Request Metrics with SourceLevel
Connect your GitHub organization or GitLab group, and let SourceLevel do the rest. We provide insightful charts and reports based on your team's daily activity.i want engineering metrics