Learn to like Pull Requests
Pull Requests can be a pain. Waiting for days to get a review only to end up with irrelevant comments, or having to spend hours reviewing a colleague’s messy work…
I used to have the same experience, and I’d like to share my journey to help you get out of this situation too.
The story
Things started changing when I joined 360Learning. Here is one of the first Pull Requests I wrote back then:
Being new there, I wanted to make sure my PRs were pleasant to read for my coworkers. I thought I had succeeded: the title was clear, I gave all the context needed, and even explained step-by-step what was implemented. However, one year later, I can see how far I’ve come.
It took 17 days and 136 comments to be approved. It was a terribly long review, and it made it impossible to give visibility to the product manager on the release date. It was painful for everyone. So, where did I go wrong?
When I opened this PR, I thought that writing good code was all that mattered. Pull Requests were merely a formality before merging my work into the main branch, a necessary evil. I was used to waiting for days before having my code reviewed, and having only one comment: “LGTM”. But that’s not how things work at 360Learning.
Pull Requests should be a blessing
Like any tool, the impact of PRs depends greatly on how they are used.
At 360Learning, code reviews are an opportunity for both reviewers and reviewee to learn and share knowledge. It allows us to grow, and it’s probably where I’ve learned the most in the past year.
But with many discussions going on in each PR, how can we avoid a painfully long review?
#1 - If it hurts, do it more often
One of our most important rules about code reviews is: Never take more than 1 business day to review a PR. This ensures reviews are done quickly, and no developer is stuck waiting for feedback. But more importantly, it allows developers to push code frequently, and as Martin Fowler explains in his article, frequency reduces difficulty.
My PR took 17 days to be approved, but it also took me 12 days to write. Had I pushed a PR for every 1 or 2 days I spent coding, things would have gone differently. It may be obvious now, but it did not seem possible at the time because my feature needed to be entirely finished.
Just because the final feature needs to be released in one go, doesn’t mean parts of it can’t be released earlier. In my case, I could have easily brought invisible changes to the code to prepare for it, like fixing a bug, refactoring some code or creating an endpoint that isn’t used yet.
If intermediate changes can’t be deployed without impacting the product in a visible way, there’s another solution: feature flags. They hide the work-in-progress so it feels like it gets released only when finished, allowing you to push regularly to the main branch.
Cherry on top, having regular small PRs instead of one final one is the best way to show the progress of a feature. It allows you to provide a plan of what you will implement in which order, and at which step you currently are.
This way, your changes can be tested more frequently and in smaller doses. You can even ask for an iso-functional review, to make sure that nothing broke.
#2 - Keep it short
The most obvious consequence of waiting until the last minute to create a PR is that it’s going to be huge. Mine changed more than 800 lines of code. I admire the determination of my reviewers at the time, because it can be tempting to do a cursory review in this situation.
To avoid that and make sure reviewers stay focused, we believe that a PR should not take more than 15mn to read. For us, that generally draws the line at around 300 lines of code changed. How can we make sure we stay under that number?
When a PR is too big, it usually means it does multiple things at the same time. Take the PR I wrote for example: the description mentions 4 different things I did to code the feature itself, and 2 things I fixed while coding it. Why not split it into 6 PRs? It would have allowed me to explain each change, to have different reviewers for each part, and to merge the small fixes without having to wait for the whole feature to be approved.
If you can’t think of any separation that would make sense for what you’re coding, don’t split one big PR into multiple ones for the sake of it. You will end up with deeply linked PRs that can’t be understood by themselves, and your reviewers will have to go back and forth between them to understand what is going on. No one will benefit from this situation. Instead, you can ask your reviewers for help on this topic. They should be more than happy to help, if it means they’ll have shorter reviews in the future. Moreover, it may be a sign that you’re modifying spaghetti code, and that some preliminary refactoring work would help.
#3 - Provide context
A PR should contain enough information to be understood by anyone in your team, be it you, your reviewers, or any dev that stumbles upon it in the years to come. It wasn’t the case here.
My title was good: it was self-describing, and included all the useful keywords. A good test is “how easily can I search for this specific PR while vaguely remembering it?”. I was able to retrieve mine in a few minutes when I looked for an example to illustrate this article. The description however is not the best: it’s too short.
The length and content of the description should match the complexity and importance of the change. For a straight-forward bug fix, a link to the original issue and a brief explanation of the bug is enough. Since I was developing a complex feature my reviewers were not familiar with, I should have detailed more.
Here’s a new, more recent example where I briefly explain the feature, and most importantly why I chose this implementation and made certain choices. It remains short to read because I took care of splitting the PR before submitting it.
#4 Rewrite your commit history
When I joined 360Learning, I only knew of one way to review a PR: read the description, then read the whole diff, and make comments there. I quickly realized that some of my coworkers had another way of proceeding.
Reading “commit by commit” means that you read each commit in the order it was published, and make comments on these commits directly. The benefits are multiple: it allows you to review smaller chunks of code, understand the history and logic behind a change, and if the commit history is well-written, it can save you a lot of time. Once I’ve started reviewing this way, it made my life a lot easier.
But no commit history is well-written from the beginning. Sometimes, I would introduce a bug and fix it 5 commits later in the same PR. Reading commit-by-commit lead my reviewers to comment on this outdated code, pointing out the now-removed bug. It frustrated them, and it frustrated me.
To avoid that, I started rewriting my commit history. It allows me to proofread my work, remove any mistake and clarify the changes for my reviewers. It feels like cleaning a math paper once the result is found: it’s important to show the intermediary steps and how you got there, but don’t show your drafts. However, rewriting history can feel wrong. Was I not taught to never use push -f?
While it’s true that the history of a repo should not be tempered with, there isn’t any reason not to tamper with a private and local branch. Merging clean commit histories actually make the history of the repo easier to read, so I was quick to set aside any reluctance.
The command I use all the time while rewriting history is git rebase interactive. It allows me to do virtually anything, from renaming a commit to re-ordering, splitting or merging them. If you’re looking to learn how to use it, I suggest the article Git Interactive Rebase, Squash, Amend and Other Ways of Rewriting History. It took me some time to master it, but it’s extremely rewarding. And remember: if it hurts, do it more often.
Now that I’ve mentioned how to rewrite history, let me give some insight on what I consider to be a well-written commit history:
- Each commit should be atomic: do one thing, and one thing only
- The flow of commits should be logical (don’t create a function before needing it, don’t introduce a bug and fix it later on)
- No commit should break tests, so they can be reverted individually
- A commit title should be short (less than 50 characters) and start with an imperative verb (fix, add, remove, merge, revert, …)
- If a commit needs an explanation to be understood, it should be in its body (by adding a break line after the title)
That’s it
I’ve come a long way since that PR I made one year ago. I’ve made errors, split PRs the wrong way, messed up while rewriting the commit history, forgotten important elements while writing a description… But this is all part of the journey, and it helped me become better at it.
It might sound daunting, but as with any craftsmanship, there is reward: your speed, code quality, but also pleasure to review will increase gradually with your mastering of the tools. And most importantly: it will enable your reviewers to make more relevant comments, allowing you to learn and take your coding skills to the next level.
If you follow this path, you’ll certainly end up looking at your old messy PRs like I did, and be glad you now know how to get the best out of code reviews.
Quick recap
A good PR should:
- Present a small amount of work (less than 2 days)
- Not depend on other PRs to be merged
- Have less than 300 lines of code modified
- Have less than 10 commits
- Be self-describing (the title and description should provide all the required context)
- Have a clean commit history…
…with each commit being:
- Unitary (do one thing)
- Coherent (no need to know the future to understand it)
- Stable (don’t break tests)
- Self-describing (use both title and commit description)
