Pull Requests that make code reviewers smile
Pull Requests (PR) are central to most software development organisations these days. Modern source control tools such as Github or Continuous Integration ones like CircleCI have helped a lot making them more accessible. Yet, it seems developers consider PRs mostly as something that you need get through to get your code merged to master. It runs your tests and the merge button becomes available.
We are forgetting that the main purpose of a PR is to receive a code review.
Someone reviewing your code is doing you a favour: They take from their time doing more fun stuff such as coding and instead try to understand what you have been trying to do. They help you catch issues before they reach production. Probably your code reviewer will save you from several rollbacks and embarrassing postmortems.
It is in your interest to make it as easy as possible to give you a good code review.
Sometimes, a PR makes me feel like closing my browser tab and not giving a damn about it, because obviously the poster didn’t care about my time. For example:
The PR has no description at all. How can I know what you are trying to fix?
Worse, the PR topic is the automatically generated one from git based on the branch name. Arghh!!
The PR has one big commit with multiple different things, like indentation fixes, some refactorings, and the actual change.
The PR is made of plenty of "Fix tests" commits.
So how can you help your code reviewers, besides obviously writing clean code? Have good commits and a good description.
Organize your individual commits cleanly. If you need to do some code cleanup before implementing your actual feature, put it in a separate commit. Your code reviewer can check the PR commit by commit, going quickly over your cleanup ones and spending more time on the part with the real changes. Git with interactive rebase and
git add -p makes it easy to have well organized commits.
An obvious requirement for this to work is to have good commit messages, which describe the actual commit content.
Additionally, each commit needs to be self contained. After each commit, the application must be functional, with all tests passing. Otherwise reviewing commit by commit doesn’t make sense.
A PR description gives the reviewer background about the changes. It doesn’t repeat the relevant comments you have put in the code, instead it covers why the change is made, and any specific things about. For example, if the change needs some special care when releasing, the poster will indicate it in the description. The code reviewer won’t have to worry about it, knowing the poster has already considered it. A PR about some UI changes will be much easier to understand if you add some screenshots to it.
A link to the corresponding ticket in the bug tracking system is also strongly advised, but this never replaces a description. Let’s face it, often the information in the bug tracking system is a bit outdated already when the code has been implemented.
So give some love to your PRs, your reviewers will be grateful!