At Braintree, we practice Pair Programming and Test Driven Development (TDD) quite extensively. We rely on these programming practices to validate our work and share knowledge. We also rely on pull requests for the same reasons. Pull requests are a great way to give and receive feedback on proposed code changes and collaborate with more teammates.
As our engineering teams have grown, we've found it useful to develop a process to make opening, evaluating, responding to, and merging pull requests more efficient. These are some of the lessons that we have found can lead us to a more productive and harmonious pull-request process.
How to write a pull request
Be contextual and descriptive: What does this pull request do?
- This sends an email when...
- This refactors how and when we generate request ids...
- This fixes a race condition when...
Provide context on why this work is happening
- Right now we have to manually send an email to (me@example.com), this automates that process...
- This race condition leads to deadlock and causes one of the executing threads to hang...
- The current code path is difficult to understand and makes changes risky...
Directly @mention team members or teams that you would like to review the pull request
- @user, I think you have the highest context on this code path...
- @team-scalability what are your thoughts on how well this implementation will scale?
- Github has a request feature that will allow you to request a review from a specific person or team
- Use Github’s new CODEOWNERS feature that allows you to specify people or teams that are responsible for code. When a pull request is opened, a review will automatically be requested from the code owners.
Be explicit about specific areas where you may want a more thoughtful review
- What are people's thoughts on changing the way the User class is initialized.
- How do people feel about the alternate design pattern in the Password module?
Communicate if the work is ongoing, or use [WIP] if it’s not ready to review
- I want to get early feedback on this implementation, the final work won't change much from this...
Add any open questions you may still have
- Should I raise a custom Exception here or no Exception at all...
- Can this be done more efficiently?
Talk about any alternative implementations you may have considered
- I considered creating a Request class for this method but felt that that might be unnecessarily complex.
If you use issue tracking software also include a link to the issue. It is often helpful to link to the pull request in the issue as well. Dual linking of the issue and the pull request can usually be automated - lowering friction is good. Remember that anyone could read this pull request, so give your reviewer as much context as possible.
How to review a pull request
Understand the context of the pull request
- Do you understand the issue this pull request is solving?
- Could you explain this pull request to someone else?
Cite your reasoning
- Is this against language norms or the company style guide?
- The solution looks like it breaks this other flow...
- This table has 4.13B records. Doing a linear search with each request could add a lot of overhead to our response time...
Use emojis to communicate tone, or use text if emojis aren’t personally or culturally relevant
- This looks great. Thanks for doing it...
Avoid rhetorical or condescending feedback
- Why would you do this?
- Can you see why this won't work?
Avoid hyperbole
- There are 1,000 better ways to fix this...
This is written communication so be extra cognizant of your tone. Focus on the code not the authors. Be nice and be constructive; remember that someone has put their time and effort into this and you are all working toward the same goal of producing elegant, efficient code.
How to incorporate feedback to a pull request
Thank your reviewer(s)
Don't take it personally
Double check to address issues; don't merge a request to review without addressing all the feedback
-- For clarity, reply in-thread to feedback, noting where and/or how you addressed it.
-- Fixed in commit a1b2c3d4
-- Broke this out into a separate Foo class in commit d4c3b2a1
Directly ping the reviewers to re-review the pull request when necessary
-- @user, is this what you had in mind?
An example of a pull request
If you use Git and Github, we hope you've found this guide useful. These guidelines work for us; incorporate them in a way that works for your own development process.
This guide was inspired by Github’s pull request guide and tries to expand on how to execute on many of their suggestions.