This is the second of a 3 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 (This post);
• Improving the Pull Requests process;

It is annoying right, someone has created some code and you need to do something with it. Can (s)he do it him/herself, now it cost my (precious) time that I can not spend on my work? We do this not to keep you from working, but we do this to either keep the current quality for the code and/or to improve it when you add new functionality (tests). The author of the Pull Request would like to get feedback on his or her work and you were one of the choosen!

But lets not go into details on that, let us focus on what to actually do with a Pull Request and how we should act on it. With a Pull Request, you can either be the Author (Creating a Pull Request, because you have written code that you wanted to be merged) or a reviewer (You are the extra set of eyes to take a look at it). This blogpost is about being the Reviewer.

Don’t make it personal

We start with the most important one: don’t make or take things personal towards others. We are all people doing the best we can, (most) probably working for the same company and thus having the same goal: Doing awesome work for an awesome software project for an awesome goal/service. So don’t make any comments to others like “You are doing this wrong.”, “You are stupid, it is like …” or “Just approve it dumbsh*t” etc. If the author is not understanding the comment you make, spend some extra time to help him/her by explaining it so (s)he will understand it. If you are working for the same company, have a (zoom)call or drop by, or have a chat personally to explain it so everyone is on the “same level”. You should never start or be part of a flaming war, this will help nobody and will only cause severe atmosphere issues in your team/community.

Understand what the Pull Request is about

So you have received an email that you were added as a reviewer, or someone have send you a link to a Pull Request, the first thing you should do is understand why this Pull Request exist and what it will solve. Check the userstory/issue in the ticket system to understand what is needed so you know how to proceed with the reviewing. If the userstory/issue only includes something like that the documentation needs to be updated, then it won’t make sense to make an comment about “the lack of tests”, but if it is about “Implement functionality x” then you will probably know that you should expect something like documentation and (integration) tests next to the code.

Understand the change

Now we know why this Pull Request exist and everything is clear for us, so we can actually review the Pull Request. With each file that is part of the Pull Request, try to understand what has been changed. If for example the documentation is updated, make sure that the documentation makes sense. Is it clear what the author is saying, do you think the targetted audience understands what is documented etc. The same is with tests, are these newly added tests useful (testing the correct “thing”) or are they added just to “satisfy” someones needs to add a lof of tests?

If things are unclear, just ask the author to clarify them.

Is it complete

Is the Pull Request complete? Is it included with the proper documentation? Does it contain tests and if tests are added make these new tests sense? If there are no tests added when you think there should be tests, just ask for it “Thank you for making the Pull Request. I do see there is new functionality added which I very like but I don’t see any tests to validate this. Can you add them?” Does it contain proper logging? When logging has been added, is the amount enough, or does it make sense. Is it something that you can use for monitoring purposes, or does it needs to be monitored?

Commenting

Now you have found something that you think that needs to be changed. What is the best way to do that? Well firstly, do not make it personally! I do hope that that was also your first suggestion ;-). And this is probably the most important and thoughest part

When you are reviewer of a public available repository, then it is a bit differently then when you are commenting on Pull Requests for coworkers. First of all, thank the author for being kind to making time to create the Pull Request. (s)he did not have to create a Pull Request ((s)he could also just move on) but did spend time on it to actual create one, so to start with a “thank you” is the very least you can do.

In one way, be direct on what needs to be changed, but also provide some information on the why it should be changed. It will show the author the reasoning on why it needs to be changed and (s)he can learn from it.

Focus on what matters

I know it might sound silly, but focus on the things that matters. Do you really want to comment on small typo in a comment in a script if it is totally clear what was meant with it? Do you really want to comment on a 2nd or 3rd empty line or anything that is related to style or view of the code? Instead focus on the actual code, does it work as you think it does. Does the tests are sufficient or do you think it can be improved. Is the documentation clear enough so the audience of the documentation understands it.

Decline is always an option

Most people don’t like it when someone declines their Pull Request. Probably because the author spends a lot of time to implement something and then someone just “decides” to decline it. But that is not the case. And in a way, you have a lot of power about this Pull Request. So don’t let it go to your head and “order” changes or decline it to show your power. I say to everyone and even when people that join my team that I follow the following rules to when declining a Pull Request (And they should too):

1. When the Pull Request does not make any sense at all related to the userstore/issue. The userstory says “a” and for some reason, the author has implemented “z”.
2. When it would be merged, there would be a possible (security) issue when it is deployed to any environment. Like when the build is running and is deployed to an environment like dev or test, that certain functionality stopped working, or that an api endpoint is openly available when there should be some form of authentication in front of it.
3. When there is no activity and progress with the Pull Request and is already open for a while. Sometimes on for example on Github, someone creates a Pull Request and then totally “forgets” it. So when changes are requested and there is no activity for the last x time, I will decline it (Where “x” time could be 4, 5 or more months).

But when you are an reviewer on a public available repository then it might just be possible that the Author has created new functionality that you maybe don’t want to have merged at all. As an example, I have several Ansible Roles that only works for the major Linux Operating systems. But what if someone creates a Pull Request with changes that it also works on a Windows host? I don’t work with Windows, let alone that I have a Windows host available to test future changes, so should I merge that Pull Request? If I merge that Pull Request, I am also responsible for maintaining it, otherwise I can not keep the quality of my code to a specific standard.

But most importantly, when declining the Pull Request always provide a proper and good reason. An “This sucks” description is not correct, but I don’t have to tell you that. But when you decline the Pull Request, have an open discussion with the author so that you are both on the same line.

Summary

So if you properly are doing reviews it might take a while to go thru them, but better spend time now before it is merged than after. As a reviewer you have a lot of control if the Pull Request will get merged or not, so don’t act like that. Work together with the Author in a constructive way to get the Pull Request merged if there are any improvements that needs to be applied.

If you have any other ethics that you have missed on this page, please let me know and I can update this page.

With the next post, we will dive in some processes to hopefully makes this whole Pull Request process a bit smoother.