3 tips to make your Git pull requests easier to review
Git is perhaps the most commonly used developer tool, so using it well is an important skill. There is definitely a lot to be said about using all the features of Git, for example, some Git aliases can save you time and do powerful things for you. However, I would argue that the most frustrating and time-consuming part of using Git is pull request (PR).
There are two actors in every pull request — authors and reviewers. When things are not running smoothly, both sides tend to put the blame on the other — the authors think the reviews are taking too long, while the reviewers think that the authors are making the PRs too complicated or are not responding to the feedback appropriately.
As is usually the case, the truth is somewhere in the middle. I think that everyone has to play their part in making the process as seamless as possible. In this article, I am going to look at both sides and draw on my personal experience on how to get there.
Since the article became very long, I split it into two parts — in this part, we are talking about authors.
The PR review process is definitely tougher on the authors — the onus is on them to get the PR reviewed and approved. Sometimes it takes a long time to get a review and it might be due to the quality of the PR. Perhaps it is too long or too messy and difficult to review. The longer the review takes, the more likely the reviewer is to get sidetracked or tired. It is in your best interest to make them as easy to review as possible.
1. Keep each PR as small as possible
I am sure most of us have heard the classic principles of object-oriented design. One of them goes as follows — classes should have a single responsibility. I think PRs are very similar in this aspect — ideally, a PR should only do one thing. By that, I don’t mean that it should only change one line — it should implement a single feature or fix. If you are using BDD, it would add or change one user journey.
While it may seem like bundling multiple small changes into a single PR would save some time, it is also true that it makes the PR harder to review. Worse, if the PR causes any problems and has to be reverted, you will have to revert all of your changes and then investigate which of the changes is causing the problem, which might be difficult to do.
Here are some ways that you can make your PRs shorter. Note that these ideas will not be applicable in every situation, but it’s good to think about it if your PR grows large:
- Make your features small. This is something that ideally should be done at the planning stage. You should avoid having a single feature that requires multiple changes in the first place. Being able to anticipate it in advance will make it a lot easier for everyone. When discussing features, discuss the proposed steps for implementation and think about whether those steps could be done as independent pieces of work. Most of the time, the answer is yes and those independent pieces should have separate tickets.
- Refactor first. If your feature requires some refactoring work, do the refactoring in a separate PR first and only then work on the feature itself. Refactoring should result in no changes to the existing functionality, so it is going to be quite difficult to validate refactoring if you bundle it with a feature, not to mention, a lot more difficult to review.
- Do not integrate new code. If your feature creates new files, whether it’s classes, components, or helper methods, they can be reviewed and merged separately. You should not attempt to integrate new files in the same PR as you introduce them. This is because you can easily merge new files into the main branch. New files will not usually result in any changes to the functionality, as long as the new files you created are not used anywhere in the existing code. Once you’ve merged the new files, you can then open another PR where those new files are integrated into the existing code.
As a side note, what is considered a small PR will depend greatly on the programming language, framework and the type of work you are doing, so I am reluctant to give guidelines in terms of lines of code. Lines of code does not necessarily correlate with complexity — you could have 1000 lines of changes that are very easy to follow and review and you could also have 100 lines of unreviewable gibberish. You will probably be able to set up internal guidelines based on other PRs in the codebase.
2. Commit often
Many engineers tend to have huge git commits, often a single commit per pull request, and I certainly sometimes find myself slipping into this bad habit as well. I find that it’s bad for 2 reasons:
- It does not help you document the code. Git commits actually work as documentation. If your commit says exactly what your code does, it makes it easier to follow during code review. It can also serve as documentation for the future — someone may be looking at your commit in 2 years’ time, and having a small commit with a descriptive message will make it a lot easier for them. Even better, it could even be you looking at your own old code! Once so much time has passed, it really does not matter who the author is — you won’t remember a thing.
- It is error-prone. Let’s say you are working on a brand new feature that is 200 lines long. Everything works great, you just need to make one final change. However, after making that change, you noticed a small issue in another part of the code. You’ve tried a few different things, deleted and rewrote multiple lines of code and now you’ve actually gone from bad to worse and your code does not even compile anymore. Undo button is not an option, since you’ve deleted and rewrote multiple lines. Had you committed the working code, you could simply revert back to that version. Instead, you might have to spend hours unravelling the mess that you’ve gotten yourself into. If you commit every few minutes you will lose at most a few minutes of work.
To make committing easier, make sure you have a Git client with a GUI that is separate from your code editor. For example, GitHub Desktop is a simple tool that has other useful features as well.
3. Don’t be defensive when addressing the feedback
Coding is a highly individual and intellectual profession and it is normal to feel a degree of attachment to your code or coding style. However, when working in a team, it is important to be ready to compromise for the benefit of clarity and consistency with the rest of codebase.
Once you get a review, you need to be prompt at addressing the review comments. You do not have to agree with every single thing every reviewer says — in most teams, you will have the ultimate ownership and responsibility for your work. However, you do need to be mindful of how you are addressing the review feedback. Once you get a reputation for being difficult to deal with, you may find it difficult to get PR reviews.
- Don’t resolve other people’s comments. Resolving comments should be done by the person who started the comment thread as only they know whether their point has been addressed. While it may seem like a good idea to keep things tidy by hiding the comments you think you have addressed, you should consider the reviewer’s perspective. If you resolve a comment without addressing the suggestion they may feel that you’ve ignored their point. If they feel particularly strongly about their comment, they might bring it up again anyway.
- If you don’t understand the comment or disagree with it — do not just ignore it. Ask for clarification or explain your point of view. If this applies to multiple comments by the same person, consider contacting them directly to go through their review. GitHub is not the best place for discussion, having too many comments will make the PR messy and less attractive to review.
- If you do address the comment and make a change, consider copying the git hash of the commit that addresses the change. This makes it easier to review the new changes. Keep it to one commit per comment, unless there are multiple comments about the same issue or the issues are closely related.
If you find yourself struggling to get PR reviews, it may be because your PRs are difficult to review, so people may deprioritise them in favour of PRs that are easier to handle. Making your PRs and commits smaller should help with that, while keeping a pleasant disposition when addressing the feedback will make people more likely to want to work on your PRs in the future.
If you feel that you are following the steps, but are still not getting reviews, it may be that your team has a problem with review culture, where engineers do not put aside sufficient time to review other people’s work. In the second part of this article, I will talk about the reviewer side of PR reviews and how it can be improved.
As a final tip, ask a colleague you are pretty close to how they feel about working on your PRs. It is unlikely that your peers will feel comfortable openly telling you that they don’t like working on your PRs, but someone who knows you well might be able to give you some pointers.
