Preparing Your Pull Request For Code Review

Anyone that is working in a team and is collaborating with others on writing applications is using some form of version control system (VCS). The word collaborating is the important one and to achieve a good level of collaboration, people usually create pull requests in their software development tool of choice, be it GitHub, Bitbucket, GitLab or one of many other out there.

Pull requests are containers for commits that you have done, communication around those commits and changes therein, a code review entry point as well as some other things.

Before Creating The Pull Request

To help the developer that is going to review your pull request, you have to start doing some things before creating the actual pull request. What those things are might depend on the agreed code style, test coverage, way you write commit messages and so on. If you are contributing to the open source project, chances are there are guidelines for contributors specified in the CONTRIBUTING file.

In any case, writing good commit messages goes a long way. I personally read all the commits in chronological order before taking a look at the actual code. Also, separating commits into logical sections helps with wrapping one’s head around all the changes. Here is an example:

  • throw an exception instead of returning boolean false
  • update tests to reflect the change
  • update docs

If you tend to do a lot of commits with messages like “wip”, “working”, “my hands are typing”, for whatever reason, you can always squash them into a meaningful commit. Read up on interactive rebase.

Writing or updating tests that cover your change is also helpful for the reviewer and there are some projects that won’t even consider the PR without tests.

Finally, do the review yourself before handing it over to the actual reviewer. I always go through my code while committing and that double-check helps me spot errors early.

Creating The Pull Request

Once your branch is ready, it has been synchronized with the master branch, commits are meaningful, tests are in place and code style checks are green, you can open a PR. Again, some projects define how a PR has to look, such as Symfony or AngularJS but for most use cases you should specify what the PR is addressing (bug, feature, improvement, documentation, …), what you did and how to review or test. If you provide the way to test with steps, this will help your QA team (if you have one) and the reviewer, and also could serve as documentation or help with creating automated tests if you haven’t provided those.

If you have any continuous integration implemented, those systems will happily report back. This also helps the reviewer so he does not have to run the test suite himself. Here is how one PR looks like with CI integrations enabled:

Closing The Pull Request

Once everything is reviewed and ready to integrate, we can proceed with merging the branch to master. Again, tools such as GitHub help us here and provide us with ways to merge and delete the branch. I personally like to Squash and Merge the branch, so that the master branch has fewer commits, and that those commits are meaningful wholes – they have the title of the PR, commit messages are listed in the commit body and should we need to revert, it is easily done for one commit.

Last but not least, delete the branch that got merged. No point for that to linger and if you need to make further changes, create a new branch and, you guessed it, a new pull request :)

Leave a Reply

Your email address will not be published. Required fields are marked *