The Four Duties of Reviewing Pull Requests

One of the greatest thrills of working in Open Source is receiving your first pull request. Learning that someone not only took an interest in your project, but took enough interest to fork the code, make a change, and then submit it as a pull request is such an incredible feeling. Receiving pull requests is amazingly fun at first but, as the number of pull requests pile up, it can quickly grow overwhelming.

I have reviewed quite a few pull requests in my time at Chef (and outside of Chef as well). This is my current activity distribution on GitHub – as you can see, pull request reviews make up a significant portion of it.

Over the years of reviewing hundreds of pull requests, I have found there are four key duties anyone who reviews a pull request must fulfill.

The Four Duties of Reviewing Pull Requests

1. Always respond to the pull request
2. Determine whether the pull request adds value
3. When it does add value, evaluate the code
4. When it does not add value, communicate it in a supportive way

Let’s go into these in depth.

Duty #1: Always respond to the pull request

One of the worst things to experience as a contributor is to open a pull request and have it just sit open with no response for months or years on end.  When someone opens a pull request on a project you maintain, you must respond, even if it is to say “no” (more on how to do that shortly). Saying “I don’t want to respond because I don’t want to say no and hurt their feelings” is not acceptable.  It hurts the contributor more to not hear anything and see the pull request just hang there. It hurts them less to be told no, given a reason why, and seeing the pull request closed.

Duty #2: Determine whether the pull request adds value

When you have LOTS of pull requests to review (when I became the lead on Habitat Core Plans there were 85 open pull requests on the repository) it’s crucial to quickly assess whether a pull request adds value to a project.  Asking a few key questions will help determine this:

• Does it fix an existing issue?  If it does, that pull request returns LOTS of value.
• Does it replicate work done elsewhere? It’s not uncommon to have multiple people open up pull requests solving the same issue (yes, it works better to have someone mention that they are working on the issue on the issue itself, but sometimes people skip that step).  In that case, you need to determine which pull request solves the issue in the best way for the project at that time.
• Does it affect in progress work? If merging a pull request will cause massive merge conflicts with existing branches (yes, you ideally should not have long lived feature branches, but these do still occasionally happen in the real world), you must weigh that carefully – will it be worth the conflicts? Or can it be delayed until the in progress work is finished?

The ultimate thing to remember for DevSecOps is that pull requests return negative value when they make contributions from current and future contributors harder. There are sometimes good reasons for this, but it’s crucial to weigh the risk of merging a pull request that will cause more difficulty in contribution, even if it is the superior approach from a purely technical standpoint. Open Source projects live and die by community engagement.

Duty #3: When the pull request DOES add value, evaluate the code

When the pull request does indeed add value, there are a few more key questions to help evaluate the code.

• Is the code functional? Does the code work the way it is expected to? Ideally, automated tests will prove this, but sometimes I also pull down the code and execute it locally.
• Is the code safe? There are two aspects to code safety – the security perspective and the context perspective. It is vital to evaluate whether the code introduces potential security vulnerabilities, especially if it pulls in a 3rd party library. It is also vital – especially with a legacy code base which has accrued technical debt – to evaluate whether the code is likely to break other parts of the project.  A fix in one part of the code may not be worth breaking other areas of the code (or may require proceeding with extreme caution).
• Is the code maintainable? Is this code that someone new to the code (or even an experienced maintainer) could come into, understand how it works, and know how to change it? Change is inevitable in software – planned or not.
• Is the code compliant with existing linters and style guides? If you have a linter or style guide, the pull request should comply with it. However, if a whitespace or style decision does not violate the linter or style guide, it is best to leave it alone.

When the answer to each of these questions is yes, that usually means the pull request should be approved. Again, if the whitespace or style decisions do not violate a linter or style guide, please just leave them alone. It can seem fun to fight about, it can seem fun to be pedantic, but that will discourage someone from contributing to the project. It is not a good experience to open a pull request that fulfills all requirements and see it rejected purely based on differing opinions of whitespace or other styling.

Duty #4: When this pull request does NOT add value, communicate in a supportive way

Again, it is absolutely crucial to respond to all pull requests, especially when it is something that ultimately cannot be merged. It is critical to communicate in a way that is supportive both of the contributor and their work. Do not trash their work – even if it’s something they really should have thought more about or talked to a maintainer about first.

• Always, always, always say thank you. You must understand that a pull request is something someone often does in their own time. They took an interest in your project, took the time to get to know how it works, and then took the time to submit a pull request. That is always worthy of acknowledgment and thanks.
• Always, always, always say why. It is cruel to leave a contributor in limbo by either leaving the pull request open or closing it without explaining why. If you do this, it is very unlikely that person will contribute again. There are very good reasons for closing a pull request without merging it – but you must give that reason when closing it.

A common question I hear is “What do I do if I give feedback on a pull request and the person who opened it does not respond?” If you give someone feedback and they don’t respond in a week or two, it is fine to close the pull request. I close these pull requests with a comment – “If you are able to address the feedback in the future please feel free to re-open the pull request, but I am closing it for now”. If the person responds but your re-review and delay will be delayed, mention it in a comment on the pull request and set an expectation on when you will be able to review it. Most people are much more understanding of this than you would expect.

For an Open Source project to not only survive but thrive, it must have a cultivated community of contributors.  Again, Open Source projects live and die by community engagement. Much of your interaction with contributors will be through GitHub issues and pull requests – it is crucial to handle every interaction with care.  It may seem hard at first, but the rewards are immense. It is a wonder to see how people use a project, enhance it, and change it into something I never imagined. Treating your contributors with care and support is not only the right thing to do, it will enhance your project’s viability, sustainability, and overall impact on the world.