Why use pull requests for team development?

Our team recently modified its development workflow to incorporate a peer review process for all code changes. We decided to do this for a couple of reasons:

1. We wanted to increase the overall quality of our code. We were fairly good about writing tests and not taking short-cuts, but even when you’re diligent and understand the team norms it can be easy to make a mistake and commit code that doesn’t follow good practices. We’ve found that the review process decreases these types of issues significantly. Its much less expensive to correct problems earlier in our workflow.

2. We are striving to deploy more frequently (multiple times per week). In order to do this we need to integrate our work streams more frequently and increase the reliability of those integrations. Along with these changes to our review process we have also invested heavily in automated testing (unit, integration, and deployment smoke tests) to provide more confidence in the quality of our deployments.

3. The number of developers contributing to our main repository has grown quite a bit over the last few years. With the increase in developers it is more difficult for the team to keep up with the rate of change. We also have developers who are less familiar with the code base and project norms who are actively contributing. Having a review process significantly increases the quality of their contributions as well as helping them learn about and become familiar with the codebase.

Git to the rescue!

Fortunately we’re using git (GitHub flavor) and its pull request feature makes reviewing and collaborating on code really convenient. It allows us to easily break code submissions into small logical requests, assign them to a reviewer, collaborate on the change, and merge into our codebase. If you’re not using a distributed version control system like git, I would recommend you investigate and consider it. We’re also using a build server to automate all of our builds, integrations, testing, and deployments.

Was it hard to get started?

We’ve been peer reviewing all of our requests for a few months and it has made a significant difference in the quality of our code. However these changes didn’t come about without some initial struggle. We had to make several adjustments to get to an efficient process. We learned that its very important to define some team norms for our pull requests and to clearly identify the responsibility and expectations of the contributors and reviewers. Without these rules of engagement, things can be really frustrating for both the contributors and reviewers. It’s really easy for pull requests to become too large, a continuously changing target, or languish without a reviewer. This makes it very difficult to collaborate on work someone has invested a lot of time and effort into.

To help with these issues, we created a set of etiquette rules for contributing and reviewing pull requests. Following these rules has really helped streamline our process. The review process has become quick and easy and we merge pull requests throughout the day.

Below are the basic etiquette rules we developed. Hopefully they will be helpful to you as well!

Contributors

Check your ego at the login prompt
This peer review process is in place to help create a higher standard of code quality. No one expects the code to be perfect (even after the review). Try to accept comments graciously and understand that they are being made to help you and the entire team. If they seem unreasonable or superfluous then contact the reviewer directly to discuss. If there is a question that cannot be resolved then a team discussion may be warranted.

Small pull requests
Requests should be small. The smaller the request is in logical complexity, the easier it will be to review and fix comments.

The request should be a small logical unit that is no more than a day of work. Ideally the amount of time spent on the request would be less than a day and fewer than 10 files changed.

Check ahead of time for questionable changes
If you are new to the project or are making changes that you’re not sure fit into the design then check with the team first. Ensure that your changes are acceptable to the team. This will save time for you and the reviewer.

Use a separate branch
Be sure to use a separate branch for all pull requests. Never work in the master branch.
To create a new branch based on an up-to-date version of master:

git checkout master
git pull origin master
git checkout -b my-pr-branch

Start branch names with your name
Start your request branch names with your name (ex. wilson-this-is-my-branch) and use dashes as separators. This makes them easy to read and identify on GitHub and in the build server.

Start new branches for new requests
Once you’ve completed a request branch for a feature, no additional new functionality should be added to that request. You should only update that branch / request to respond to comments and to help with getting that request merged into the master branch. This will help to keep the requests small and makes it easier to review and merge.

If you have a new request to work on or have been asked to fix things outside the scope of your current request then a new branch and pull request should be created. This will keep pull requests from growing unmanageably large.

Dealing with Pull Request Dependencies
If your new pull request is not dependent on one of your outstanding requests, great! Then you can just create the new pull request off of master as you did above.

However, if your new request is dependent on one of your own outstanding requests (or even an outstanding request from someone else) then you can just create your new request based on the current one:

git checkout parent-request
git pull origin parent-request
git pull origin master
git checkout -b child-request

Wherever you pull the branch from, you will want to keep it up-to-date. It’s always a good idea to keep it in sync with the upstream repository.

Keep your outstanding requests current
Make sure to regularly update your branches from any upstream changes and keep the branch up-to-date on the origin. This will usually be the origin master branch but could also be a parent branch that you used as the basis for the new branch.

git checkout my-branch
git pull origin master

Ensure your branch can be merged
Before you issue the request, be polite to the reviewer and make sure GitHub indicates the request can be merged.

Ensure your branch compiles and passes tests
Before issuing a request, make sure it builds and all the tests pass on the build server.

Reviewers

Be Kind
The peer review process is intended as a way to improve code quality, not make others feel bad. Please be constructive when making comments or asking for changes.

Only comment on things that are important
This requires judgement on the part of the reviewer. In general you should be pointing out things that are significant problems with the commit (ex. unnecessary references, not following proper design, missing tests, ignoring exceptions, etc). It’s very subjective, but the review process should point out things that are critical to code quality and future maintainability. It shouldn’t be a pedantic exercise that points out small style differences between developers.

Always comment on things that are important
Make sure to comment on issues you find in the code that are important. You’re not helping anyone (team, contributor, or yourself) by merging in code that is going to cause problems later.

Respond to requests quickly
Try to review any requests assigned to you within two hours or assign them to someone else who is available to review. Quick turnaround is critical to keeping the master branch as current as possible and making things easier on contributors.

Make sure the branch can be merged
The contributor should have checked this before the request was submitted, but it’s a good idea to double check.

Make sure the branch compiles and passes tests
Trust but verify.

Follow up in Slack
It’s a nice gesture to let the person know the result of the review in Slack. They should get a notification, but Slack is a nice way to close the loop.

Delete the branch
Once you complete the merge, GitHub will provide an option to delete the merged branch. Please delete the branch and keep the repo nice and tidy.