Before we jump into what can you do to ensure that pull requests (PRs) add value to your projects instead of being a bottleneck, let's cover the basics.
What are Pull Requests?
Pull requests let you tell others about changes you've pushed to a branch in a repository on GitHub. Once a pull request is opened, you can discuss and review the potential changes with collaborators and add follow-up commits before your changes are merged into the base branch.
I've taken this definition from Github docs and you can refer to it here. Let's dive a bit deeper into it.
When you are working with a team of developers or contributing to an open-source project, it is important to keep everyone on the same page. PRs are a great way to do that. After you are done working on a feature/fix in your own branch, you open a pull request, wherein your code is available for other code owners to go over and share their thoughts with you. This helps ensure that your code is doing what it is expected to do, as well as meet the coding standards before it is merged to the master branch.
Other than getting a review from other code owners, you can also integrate your repository with a build process to ensure that the branch builds alright, all tests pass, code style is maintained throughout the process, the code coverage meets the standards, etc. There are also some SaaS tools available out there for intelligent recommendations like AWS CodeGuru.
Why are PRs important?
Pull Requests (PRs) are the perfect way to ensure that the code meets the feature requirements and the coding standards before it is merged. This protects the master branch from any "bad code" being merged to it.
But that's not it. PRs are also a great way to learn and teach. When you are asking someone to review your code, you are not only asking them to check whether your code is doing what it is expected to do but also asking them to share their opinions on your approach. Similarly, when someone is reviewing your code, they get to learn from your approach and see if they can adopt it too. Thus, PRs are a great way to share your knowledge as well.
The Problem
When not done right, PRs can be a huge bottleneck and can slow down the delivery process. There are times when you are waiting for someone to review your PR before you can merge and start the next task. There are also times when the reviewer is in a rush to go back to their tasks and decides to just approve the request. Then there are those times when the reviewer needs to review the PR over and again, and over time decides to let it through πͺ
As you can imagine, these practices can potentially disrupt projects and it is very important to work to fix it. So let's jump to do that.
The Solution
Here are a few recommendations that have worked for me and my team to make the most of the PR process.
- Big PRs with multiple changes slow down the process of reviewing. For starters, when someone sees that you have changed 1000 lines in 25 files, it just puts them off even starting the review. What can you do? Break it down into smaller PRs. You may argue that your changes are all related and cannot be merged to
master one at a time. That's fair enough. But there's a solution for that - Break it down to smaller PRs against your own branch and get that reviewed. Take a look at this GitFlow for clarity:
Let me break it down. You can create small PRs against your own branch and get that reviewed. Once approved, you can merge that into your feature branch, and work on the next feature. In summary, you are treating your feature branch as a master branch and creating PRs against it. This way, you've broken that 1000 line PR into smaller PRs while keeping it away from the master branch. In the end, you will still have that 1000 line PR, but it will only be an amalgamation of the smaller PRs which were already approved. So the reviewer would not find it as difficult anymore to review this one. π
- Add all the relevant information that you can to the PR. This can include information like the Jira ticket link, a Wiki link to the docs that you had created/had access to that are relevant to the ticket, documentation around any new dependencies you are introducing, link to reference implementation that you think is relevant, etc.
This does not need to be left to the PR Author's discretion and you can standardize it across the team. I recommend using a PR template to do this.
A suitable title for your PR can also give clarity to the reviewer before they even read the description. Adding a title like new feature or quick fix does not help the reviewer since it delivers no relevant information. Instead, add the ticket number that this PR covers and a short title describing what the task is like New tables for promo code service. Again, you can enforce this within your team also by using a tool like PR Link that checks that the naming convention is followed.
I cannot emphasize enough how powerful webhooks are. Other than getting a review from other code owners, you can also integrate your repository with a build process to ensure that the branch builds alright, all tests pass, code style is maintained throughout the process, the code coverage meets the standards, etc. There are also some SaaS tools available out there for intelligent recommendations like AWS CodeGuru. These webhooks can be added to your branch protection rules and they all need to pass before you can merge your PR. Needless to say, wait for your prechecks to pass before you ask someone to review your PR. Fix everything that these prechecks point out first and then ask for more recommendations π
Okay, that's enough about what you can do as the PR author. Let's talk about what you can do as a reviewer. Remember that it is just as important to be an efficient reviewer as it is to be an efficient author.
If you are reviewing someone's PR and have questions, ask them in the PR, not in person (like that's an option in 2021 π), not on Slack, or any other forms of communication. When you ask them questions in the PR, it is available for other reviewers to see as well. They might have the same questions and it saves everyone time to just ask the question once.
If you are suggesting a better way of doing something, explain how is it better. Don't say extract this into a method.... The author may assume you are imposing your preference. Instead, explain the reason behind your suggestion and if possible, give them a reference for your source too. So taking the same example, you can say Extract this into a method since I can see you are using the same logic in multiple places. I would advise following the DRY principle. You can read more about it [here ](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself)
Be kind in your criticism. PRs don't need to be a battleground to prove who is a better developer. It is a place to collaborate and learn. Well, this suggestion is applicable while criticizing anything. π
Finally, something outside of the PR review process. While merging your PRs, you can enforce that everyone follows squash and merge. Our commit messages are not always clean and that's okay. We have all had commit messages like "final commit", "final final commit", and "please work". These commit messages do not need to go to the Github history and that's exactly what squash and merge does for you. π
That's all I have to suggest. If there are any other practices that you follow to optimize your PR process and would like to recommend others to follow too, please leave them in the comments.
