The ethics of Pull Requests, improving the Pull Requests process

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:

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.

The ethics of Pull Requests, being the “Reviewer”

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:

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.

The ethics of Pull Requests, being the “Author”

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

It is annoying right, not commiting on the master|main branch and that you should work on a branch, create a Pull Request that people should review before it can merged into the master|main branch. But it all has a good reason, but this process just takes a little bit more time to get your code merged into the master|main branch. We do this not because it is cool or a fun thing to do, but we do this to either keep the current quality for the code and/or to improve it when you add new functionality (tests).

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 Author.

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 “This is way over your head”, “You are stupid, it is like …” or “Just approve it dumbsh*t” etc. I know, I exaggerate a bit but you understand what I mean right? If a reviewer is not understanding the pr/change/etc, 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 with either people leaving or just stopping to work at the project.

Don’t fall in love with your code

You probably did all the best you can do on this piece of code you have written. You might think it is your best work, which could be, but keep in mind that a 1000 roads leads to Rome. This is also the case with code. You implemented 1 way of doing something, while there might be different ways to “get it done”. Just because you think it is your best code “evar”, it does not mean that it is the actual best way and can not be improved at all. You are most probably wrong. You wrote the code with the best intention, based on your knowledge and experience you have on the given subject. A reviewer might have more or different experience and knowledge about the subject and is able to give new insights for your code to improve it. Be open to these suggestions that the reviewers are giving and where needed, have a direct contact/chat with that person to explain/discuss it. When you are open to these comments, you are able to learn from it and will help your (future) coding practices.

This also applies when someone declines your Pull Request. Don’t go mad or insult the reviewer. (s)he is doing just his/her job and lets be honest, you asked for their input! But when the Pull Request is declined, have a chat/talk with the reviewer and discuss it properly. Try to understand the reasoning and try to resolve it correctly. If after the discussion the conclusion was that the reviewer was right, no worries and try to apply the reviewer comments and try again. If after the discussion the reviewer was wrong, no worries again and recreate the Pull Request. But when you have recreated the Pull Request, add a small comment with a (short) summary about the earlier decline so it is clear for everyone what the result is.

Reviewer is not always right

Just because a reviewer is making a comment about something, doesn’t mean (s)he is automatically right. (s)he does this with the information (s)he has at that moment and makes a comment about it. (s)he does not know anything about the past while you were developing the functionality that have lead to what you have right now. And maybe your Pull Request was the nth Pull Request that the reviewer was reviewing, so maybe (s)he was mixing things/knowledge with other Pull Requests? But in any case, talk/chat directly with the reviewer to sort it out and when you concluded things together, update the Pull Request with a comment with this information so everyone knows about it.

Do make sure the why

And this is something you should start with, create a good and understanding description of what the purpose of the Pull Request is. Provide for example a link to the userstore/issue with some information. When you have Jira and Bitbucket for example, you can create a branch in a git repository that is automatically linked to the userstore/issue so even from the name of the branch it is automatically clear on which issue you are working on.

When you are working on a public available repository then you only have access to the “issues” part of f.e. Github/Gitlab. Check if there is something like a contribution section in the readme or maybe there is a specific document for it. Most of them explains what is needed to be done to be a contributor to the repository, like creating an issue with a proper description and with reproducing steps (In case of an issue). Make sure that it is all clear for everyone and then create a Pull Request and make sure to link it to your earlier created issue, or the issue that you picked up to provide a fix.

So really make sure before any reviewer starts doing reviewing that for everyone it is clear why this Pull Request is created and what it does solve.

Do make your own comments

There is nothing wrong with making comments on your own Pull Request. And with this, I mean that you can make a comment on why you have taken this approach or maybe add some background information that would help the reviewer to properly evaluate the Pull Request. While you were developing or working on the change, it might be that you have created several different attempts to “get it done” before you went to the solution you have right now. With making a comment as an Author on your own Pull Request, you can describe these tasks and it helps the reviewer understanding why you took “this” approach so they won’t have to ask you.

Summary

There is also maybe the downside of doing Pull Requests, you can create 1 small Pull Request that took many hours or maybe days to complete, because you have tried different things which didn’t work. A reviewer will not see this, because (s)he only sees the end result and not the things that have lead to this Pull Request.

And that is why I suggest to focus on the earlier mentioned ethics, make sure that the reviewer knows why the Pull Reqiest is created, be open for suggestions from the comments from reviewers and comment on your own Pull Requests on the areas you will know that either helps the reviewer or you will know for sure someone will comment on it. This will make the whole Pull Request procedure a lot easer for everyone.

One of the most important aspects of working with Pull Requests is proper communication between the author and the reviewer(s). Be open to each other and respect each other opinions and keep in mind that the focus is on keeping the quality and not someones ego. Don’t use the comment section as a chat application but just contact the reviewer directly if there seems be a discussion going on. And again, don’t make things personal. It helps nobody if you make things personal and it will only work in the opposite way.

What do you think what are the important ethics for the author that I missed. Please add them in the comments below and I will update this blogpost.