This is the 3rd and the last part blogpost series about Pull Request reviewing. I am writing this so I can give you my personal view on each side of the Pull Request saga, something I explain to every member joining my team. It contains about the following 3 parts:

• Ethics for the Author;
• Ethics for the Reviewer;
• Improving the Pull Requests process (This post);

We have discussed some aspects about being the author and reviewer when it is about Pull Requests. But are there any ways to improve the whole Pull Request process, so we can focus on what really matters. Like we want to merge quality code that brings functionality that someone wants to make use of it. But what ever we do, we still have to create an PR as being the Author and/or as a reviewer we still has to review it. But maybe we can do some things to make it easier for both parties even before we create a Pull Request.

Small changes

Making small changes in a code base are much easier to review. Not that you need to create lets say 5 Pull Requests with each 1 line changed, especially when these 5 lines together needs to work together for a specific funtionality. But when you are working on something big, try to think about splitting the work in smaller pieces. Think about if making something like a toggle to enable/disable the new functionality can work, so you can spread the work over various Pull Requests and knowing that your new functionality is not yet used. So eventually you have to enable it, but you can work more easily between all the pull requests and provide feedback to the rest of your code. And 5 smaller Pull Requests are much easier and faster reviewed than 1 big Pull Request.

precommit hooks

This is a bit technical, but it helps both you and the reviewer to focus on what matters. The functionality you want to add. Wehen the author is doing an “git commit” with precommit hook(s), several scripts and/or commands are executed on the files that have been changed. This can be for example linting commands that does some static analysis. If the linting fails, then the “git commit” also fails and then you can fix the issue and try again.

For an example, when you write Terraform code there is a tool called tfsec, which allows you to do static analysis for possible security related issues. If you want to create an Security Group in AWS and provides a cidr_range of “0.0.0.0/0” it will complain and fail the “git commit” execution, because it is widely open. When this happens, you can check and either update (Because it is always easy to just use “0.0.0.0/0” and not think about a smaller subnet) or validate again that this it is actually correct and add a line above with that it should be ignored (For example: “#tfsec:ignore:AWS009”). Once done and when you execute a “git commit” it will succeed.

This was just one example of possibilities with using pre-commit hooks, there are of course a lot more.

Pair programming

This is not for everyone, especially when everyone have to work from home during a pandamic and not everyone – especially in the beginning – likes that someone is looking “over the shoulders” to see you code. People will get nervous if they are “being watched” while coding and probably it adds another bit of extra pressure because you don’t want to make mistakes. But making mistakes are fine, you have a buddy that is that extra set of eyes to help you code, spot possible issues and you can discuss very easily together on the approache to take. As you are both working on it, you will both know what, how and why this new piece functionality is created.

When you create a Pull Request you can add either a comment or update the description that you both worked on it. This will let the reviewer also know that there was a 2nd pair of eyes on it and most of the actual review work was already done. So it then is a bit of a formality to approve the Pull Request. But if it is just a formality, why not commit into master|main?

Well, I think you should never merge into master. Always create a branch, create a Pull Request and merge that. When commiting to master|main directly the functionality is only known to both you and you both can still make errors. When you also have tests that are executed as part of the CI process with feature branches, you have an extra confirmation that you won’t break anything. And what I also think is very important with creating a Pull Request, it shows to the rest of your teammembers what you both have worked on. Teammember x knows that your are working on functionality y and is just be informed (I like knowing on a high level what my teammembers are working on, we are a team right?) or maybe (s)he has working on similar functionality that could affect him/her.

So this is the end of my 3 post of the ethics for Pull Requests. Are there any processes you missed on this page that I forgot? Please let me know in the comment and I will gladly update this page.