We create a lot of Pull Requests here at Sparkbox. Adam recently wrote about Code Reviews with GitHub Pull Requests and how they are part of our normal workflow. I make a change to a project, I submit a Pull Request (PR), get some feedback and an OK, and then it gets merged into the codebase. This is how we manage code changes and code reviewing. The goal of this article is to give you some tools to create these PRs in a way that makes the process of reviewing and merging them a pleasant experience for the reviewer.
What is the PR Fixing?
Is this PR fixing a bug? Is it a new feature? Why is this work being done? Having good commit messages will aid in the reviewer seeing what is trying to be accomplished. I also recommend putting in a link to the issue (simple with GitHub ex. fixes #123), or BaseCamp discussion, or something. Having a reference to the issue makes navigating the PRs in GitHub much easier to do. It’s like Hansel and Gretel leaving breadcrumbs from the problem to the solution. This not only helps the reviewer to know what the original issue/feature request was, but it allows him or her to ensure that the original issue has been solved by the proposed solution.
Do One Thing
What is this PR doing? It is much easier to accept/reject/comment one thing at a time. If the PR is doing too much, maybe it should be separated into multiple requests. I find that when I’m typing a commit message, the word and generally means that this commit/pull request is doing too much. Consider splitting it up into separate requests.
Rebase All the Things
If you’re not the only developer on the project, there’s a good chance that other changes have been merged into master while you’ve been working. It’s a good idea to rebase your code to make sure that what you are submitting includes the most recent changes. In the course of adding a feature/fixing a bug, I tend to make a lot of commits. While this is helpful in the fix of that issue, in the scope of the project, it may be better to have one commit in the history, like “feat: Adds homepage slider.” So, when I’m doing my rebase before I submit the PR, I will most likely do a “git rebase -i master” (interactive rebase). This allows me to combine my commit messages for this feature into one final message.
It’s All in the Name
The title you choose can make a big difference. “Fixes detail page scrolling issue on Firefox for Windows” is a great title, if the issue only affects the detail page... in Firefox... on Windows. The title puts the reviewer in a frame of mind that this is a browser inconsistency, when in fact, that may have just been the browser you were using when you found the issue. Details in the title are a huge benefit, but they also need to be correct. Make sure the PR title accurately describes the problem, not too narrowly and not too broadly.
Where is the Issue?
This is a good one for large projects. While having a screenshot of the affected area in the original issue is really helpful, it might not be enough to let the reviewer know what page(s) this issue was happening on that now it might not be. Let them know where this issue was happening without having to go on a treasure hunt. Drop in a URL or instructions on how to get the state of the app/site to the place where the error was occurring. Any steps necessary to reproduce the problem would also be helpful.
Help Me Git It
If you need a little help better understanding Git, there is a great course on How Git Works to help you do just that.
Go forth, commit, and PR with confidence that you will make someone happy.
