How to Tame Your Pull Requests | Crit Russell

August 5, 2019

How to Tame Your Pull Requests

These guidelines were used in a talk I presented at the Idaho Falls Programmers Group on July 17th, 2019.

IFP Meetup 2017-07-12


Context

A Pull Request (PR) has at least two participants.

  • An author
  • A reviewer

It makes the most sense in a team environment.

As The Team

  • We care. The rest of the business can be a burning dumpster-fire but we are the cool, professional core that delivers amazing features consistently and on time.
  • We have each other’s back. No one else understands what we have to do so we can only rely on each other for quality.
  • We communicate outside of the PRs. We talk about policies. We reach an agreement on general principles, write them down, and train new staff on those principles. These are living documents that will change over time.
  • Our Continuous Integration (CI) system handles code policies and principles. In a mature product, we don’t need to talk about formatting. We have a CI system to run formatting tools and code vetting.
  • We never need to pull down and run unit/integration tests. Our CI system does this for us.

As The Author

  • I am not my code. I check my ego at the PR.
  • My PR should help the reviewer understand the problem I’m trying to solve or the feature I’m trying to create.
  • My PR should pass our CI tests/vets before I make it available for review.
  • I keep the changes on-point and don’t add any that are out of context to the feature request. If I run across out-of-context changes, I make an issue in our ticket system (or whatever we use to handle backlog), and then move forward with the feature.
  • I add PR comments to lines of code that I feel would help the reviewer more quickly understand the change and as opportunities for larger discussions about our policies/principles.
  • This is a “public” interaction and the rest of the team is watching. I’m trying to expand everyone’s understanding of what our project does when my PR is successfully merged.

As the Reviewer

  • I am not judging the author. I have their back and are their support. My job is to understand the problem and make sure that the end result is something the entire team can get behind. While I am not my code, our team is our (collective) code.
  • I am always trying to help the author with my comments. Teaching moments are ok but I have to be considerate of my tone. If I’m ever in doubt about how my feedback will be handled by the author, I do it in person and not in the PR. Text is really easy to misunderstand.

© Crit Russell