PR

Write your own pre-commit hooks

/ Werner Dijkerman / 1 Comment

In one of my previous blog posts I described that using pre-commit hooks makes life easier, it helps you writing better code. When you want to commit your changes, you immediately get a result if your code has met the various criteria the owner of the repository has set. When you google around, you will find various scripts that you can make use in your own setup. But what if you can not find that specific one? Then you will need to create your own hook.

I had to do this as well, not because I had a rare case to solve, I just wanted to learn how to write a hook myself so I can easily expand my own pre-commit library. And it seems out to be very simple. 🙂

Use case

My very basic use case: I want to validate a yaml file and make sure that it is properly formatted.

I found a very basic tool that helped me format an yaml file, named “yamlfmt“. I need to use this tool in a bash script, which you can see here:

#!/usr/bin/env bash
# Will pretty print YAML files.

if which yamlfmt &> /dev/null $? != 0 ; then
    echo "yamlfmt must be installed: pip install yamlfmt"
    exit 1
fi

EXIT_CODE=0

for file in $@; do
  yamlfmt ${file} -w
  if [[ $? -ne 0 ]]
    then  EXIT_CODE=1
          echo $file
  fi
done
exit $EXIT_CODE

First we need to make sure that we check that the tool is installed, otherwise we print an statement on how the user can install the tool. We need to have that tool installed, so we immediately exit the script with an exit code of 1. With that, the “git commit” action will also fail and thus the user needs to take action to install the tool before it can try to commit the changes.

The last part of the script is doing a for loop and will run the “yamlfmt” command on each file that is passed as an argument to the script. With each pre-commit hook, all of the files that are part of the commit are passed as an argument to the script (Not entirely, but we will discuss this a bit further on ;-)). We collect the exit code of the “yamlfmt” command and checks if that will not equals to 0. If that happens, then there will be an issue with the yaml file and we print the name of the file to stdout.

But how does the script know that it should only do yaml files? If we have a text file or png file, this will fail!

First, we need to add this script into a repository that contains other pre-commit hooks scripts. If you don’t have one, or don’t have other scripts is also fine. I have a dedicated repository for that, which you can find here https://github.com/dj-wasabi/pre-commit-hooks. This repository contains all the pre-commit hooks that I use in 1 or more (public) available repositories. We just need to have a git repository where we can store this script and most importantly, we need to store a “.pre-commit-hooks.yaml” file. This file will contain some information about the scripts which we can use in the rest of our repositories.

In order to get the above menionted script work, we store this script inside the “bin” directory in the git repository and name the file “verify-yaml.sh“. It doesn’t have to be specific in the “bin” directory, you can also just place it in the “ROOT” or some other directory, whatever you please. But in the “ROOT” of the git repository we will have to create the “.pre-commit-hooks.yaml” file and we will include the following contents:

- id: verify-yaml
  name: Pretty Print YAML files
  description: Checks YAML files and pretty prints them
  entry: bin/verify-yaml.sh
  language: script
  files: \.(ya?ml)$

There are 2 important keys that requires some additional information (I won’t have to tell you that the “entry” key is the location to the script right? Oh wait, I just did. :)) These are the “id” and the “files“.

The “id” is the value that you need to use in your repositories where you want to make use of the pre-commit hooks. Like with the https://github.com/dj-wasabi/dj-wasabi-release/blob/main/.pre-commit-config.yaml you will see the following:

repos:
- repo: https://github.com/dj-wasabi/pre-commit-hooks
  rev: master
  hooks:
  ...
  - id: verify-yaml
  ...

So every time I do a commit in my “dj-wasabi/dj-wasabi-release” repository, it will execute the pre-commit hook with id “verify-yaml“.

The “files” key in the “.pre-commit-hooks.yaml” entry is when the script should be executed. We only want the script to be executed when the file is a yaml file and not with a text or png file. The “\.(ya?ml)$” makes sure that it only affect files with the file extention “.yml” and “.yaml“.

Thats all folks!

It looks very easy right? Yes it sure is and when you start to work on your own hooks, you will create more. Because I am in a writing mood right now, lets take a look at the following script that I wrote:

#!/usr/bin/env bash
# Do not allow commits on provided branches.


EXITCODE=0
while getopts b: flag
do
    case "${flag}" in
        b) BRANCHES=${OPTARG};;
        *) echo "Unsupported option provided."
           exit 1;;
    esac
done

for BRANCH in $(echo ${BRANCHES} | sed 's/,/ /g');
    do
        if git rev-parse --abbrev-ref HEAD | grep -e ${BRANCH} > /dev/null 2>&1
            then  echo "You are not allowed to commit on ${BRANCH} branch."
                  EXITCODE=1
        fi
done
exit ${EXITCODE}

I don’t want to commit my changes on “master” or on “main“. And yes you can configure in most cases on the remote site (Read: the Git Server, like Bitbucket or Github) that it will not allow pushes to “master” or “main“, but I just want to prevent the commiting to actual take place. Everytime when I commit on a branch that the remote server is not accepting, I have to google for “git commit undo” and look for the command to undo my commit (Because for some reason I can not remember the git undo command :)).

Once I have undo my commit, I will create a proper branch and do the commit and push again. So if I can prevent committing to “master” or “main” with a pre-commit hook, my life will get easier because I don’t have to undo my commit etc! (Wait, wasn’t that the title of the blog post that I have written before this blog post? ) 🙂

If you have any question and or remarks, please let me know. If you have written a hook yourself and you want to show it, let me know as well!

May the hooks be with you!

Advertisement

Using pre-commit hooks makes software development life easier

/ Werner Dijkerman / Leave a comment

Pull requests, you either love and see that they do provide benefits in the way of working, or you dislike them and see no purpose in them at all. I discussed the Pull Requests process in these “Author“, “Reviewer” and “Process” blogposts, so check these out as well. No matter which side you are on, but I think you want to create quality code and be consistent with each change. And if you are like me, then you would expect this as well from your teammates/co-workers. But how can we make that happen?

Pre-commit hooks can help with that. A pre-commit is one of the several hooks that are available in git that we can use to execute scripts, while running an git command. A full list with hooks can be found on this page https://githooks.com/. 

With a pre-commit hook, we can execute scripts as part of the “git commit” command. If the exit code of all of the scripts result in a zero (successful execution) then the commit will take place. If 1 or more of the scripts fail, the commit will be unsuccessful and fails to complete. Once that is happening we need to resolve the issue and try again.

When you have a git repository somewhere cloned on your host, you are already able to use pre-commit hooks or any of the other hooks that Git has. In the “.git/hook” directory you will find a bunch of “.sample” scripts. You can use these as an example and if you remove the “.sample” in the filename, then the script is active and triggered when the stage is executed.

$  ls -l .git/hooks
total 112
-rwxr-xr-x  1 wdijkerman  staff   478 Jun 14 21:01 applypatch-msg.sample
-rwxr-xr-x  1 wdijkerman  staff   896 Jun 14 21:01 commit-msg.sample
-rwxr-xr-x  1 wdijkerman  staff  3327 Jun 14 21:01 fsmonitor-watchman.sample
-rwxr-xr-x  1 wdijkerman  staff   189 Jun 14 21:01 post-update.sample
-rwxr-xr-x  1 wdijkerman  staff   424 Jun 14 21:01 pre-applypatch.sample
-rwxr-xr-x  1 wdijkerman  staff  1638 Jun 14 21:01 pre-commit.sample
-rwxr-xr-x  1 wdijkerman  staff   416 Jun 14 21:01 pre-merge-commit.sample
-rwxr-xr-x  1 wdijkerman  staff  1348 Jun 14 21:01 pre-push.sample
-rwxr-xr-x  1 wdijkerman  staff  4898 Jun 14 21:01 pre-rebase.sample
-rwxr-xr-x  1 wdijkerman  staff   544 Jun 14 21:01 pre-receive.sample
-rwxr-xr-x  1 wdijkerman  staff  1492 Jun 14 21:01 prepare-commit-msg.sample
-rwxr-xr-x  1 wdijkerman  staff  3610 Jun 14 21:01 update.sample

But we are focussing on to pre-commit hooks and there is an easier way to maintain the pre-commit hooks. Like you already have seen, we can only use 1 pre-commit file, and we want to use a lot more than just one. Sure you can call other scripts, but that makes the maintenance a bit more complicated. Especially when you have more than just a single git repository. There is a package that helps us to have a more maintainable soultion with regarding to pre-commit hooks and using them.

Installation

We have to install it first and we have to install it on our development workstation/laptop. Make sure you have Python and Pip installed before executing the following command:

pip install pre-commit

This will install the pre-commit application on our system.

“That is all nice, but I have no idea what kind of scripts we can execute as part of a pre-commit?”

– you?

No worries, lets use an example. This Github repository https://github.com/dj-wasabi/dj-wasabi-release/ contains some Python scripts that I use for maintaining my Github repositories. We will not discuss these scripts, but there is 1 important file in this repository and is named “.pre-commit-config.yaml“. You will probably find this file also on other repositories that are on my Github account. This file contains all the scripts that will be executed when we do a “git commit”. Lets take a look at the file.

repos:
- repo: https://github.com/dj-wasabi/pre-commit-hooks
  rev: master
  hooks:
  - id: shellcheck
  - id: markdown-toc
  - id: verify-yaml
  - id: no-commit-on-branch
    args: ['-b master,main']

It contains a “repos” key which is a list, this means you can have multiple entries configured in the file (As this is currently the case). Each of these entries needs the following 3 properties to work properly:

  1. repo: The git location where the scripts are stored;
  2. rev: The version of the repository, can be a tag, branch or git commit;
  3. 1 or more hooks, which has 1 or 2 keys: id (and args).

In the code block above, we see that there is a Github repository mentioned, namely https://github.com/dj-wasabi/pre-commit-hooks and on master we have several scripts available. The pre-commit hook will see that in this repository a file named “.pre-commit-hooks.yaml” exist. This file knows exactly what to do when the pre-commit hook is executed with the provided id’s. In the “.pre-commit-hooks.yaml” file, there is a configuration for the “shellcheck” id.

- id: shellcheck
  name: Shellcheck Bash Linter
  description: Performs linting on bash scripts
  entry: bin/shellcheck.sh
  language: script

The pre-commit hook now knows, that with the “shellcheck” id a script “bin/shellcheck.sh” in the https://github.com/dj-wasabi/pre-commit-hooks reposity needs to be executed. But in all fairness, you are not ready yet. 9 out of 10 times you will need to check the README on the mentioned repository to see if you need to install any other tool to make the script work. Because I am working on a Mac, I do need to make sure that shellcheck is installed:

brew install shellcheck

And I need to make sure that the dependencies that the other scripts in the “.pre-commit-hook.yaml” uses are installed on my Mac.

Once I have done that, we need to tell our “.git/hooks” directory that we want to make use of the pre-commit hooks. We will do that by executing the following command in the root of our git repository:

pre-commit install

This will give us the following output:

$ pre-commit install
pre-commit installed at .git/hooks/pre-commit

You can do now an “ls -l .git/hooks” and compare the output with before. When you execute a “git commit” from now on in this repository, it will execute the pre-commit hooks. Remember that for each git repository you have cloned on your host, it will contains their own configuration.

But why?

“But I still don’t know what kind of scripts I can execute as part of a pre-commit hook?”

– you?

The problem I see and/or have experienced a lot is that when people gets invited to be a reviewer on a PR, is that they focus on the small nitpicky things that don’t really that much. Things like, wrong identation, to many empty lines, missing space or trailing space (well, I do find this one annoying ;-)). They should be focussing on the actual change like I mentioned in one of my earlier blogposts. And 9/10 times you can prevent things like this by executing a linter or formatter. This linter will provide an overview of warnings and errors on what needs to be fixed before the linter is happy, so an ideal candidate to execute it as a pre-commit hook. So with pre-commit hooks we can sort of prevent “garbage” to be committed into a Git repository.

When you write for example Python scripts, like what I do with the https://github.com/dj-wasabi/dj-wasabi-release/ repository, you can make use of the “flake8” tool and with every “git commit”, the linter will be executed and provide errors. And that can be achieved by adding the following few lines in the “.pre-commit-hooks.yaml“:

- repo: https://gitlab.com/pycqa/flake8
  rev: 3.8.4
  hooks:
  - id: flake8
    additional_dependencies: [flake8-typing-imports==1.7.0]

If I now do a “git commit” (or when you just want to execute it without doing an actual commit: “pre-commit run -a“):

$ git commit -am "Some message that I know that it will not make it to the log"
Shellcheck Bash Linter...................................................Passed
Generate Markdown toc....................................................Passed
Pretty Print YAML files..................................................Passed
No commit on master or main..............................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

lib/djWasabi/git.py:59:1: E302 expected 2 blank lines, found 1
lib/djWasabi/git.py:79:22: E231 missing whitespace after ','
lib/djWasabi/git.py:80:52: W291 trailing whitespace
lib/djWasabi/git.py:83:53: E231 missing whitespace after ','

Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing lib/djWasabi/git.py

Check for merge conflicts................................................Passed
$ echo $?
1

Look, the pre-commit “Failed” (Or succeeded, depends on how you look at it ;-)) and it prevented to do the actual commit in git. You can see that there where 4 lines in the “lib/djWasabi/git.py” file that needs to be fixed. But you will also see a 2nd script has failed, namely the “Trim Trailing Whitespace“.

A pre-commit script can also update files when needed (which isn’t a bad thing!) and not just fail on when certain criteria is met. The “Trim Trailing Whitespace” script has updated a single file “lib/djWasabi/git.py” and removed the trailing space. When a script updates a file, the script should exit with an exit code of 1 and provide info on what file has been updated. If I would run the git commit again, then you will see that the “Trim Trailing Whitespace” is “Passed” and that the “flake8” script only has 3 errors:

Shellcheck Bash Linter...................................................Passed
Generate Markdown toc....................................................Passed
Pretty Print YAML files..................................................Passed
No commit on master or main..............................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

lib/djWasabi/git.py:59:1: E302 expected 2 blank lines, found 1
lib/djWasabi/git.py:79:22: E231 missing whitespace after ','
lib/djWasabi/git.py:83:53: E231 missing whitespace after ','

Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
Check for merge conflicts................................................Passed

Nice right?

Updating files as part of the pre-commit hooks are very common and I do make use of it a lot. When you work with Terraform, you will probably know the “terraform fmt” command as well. And I do too make use of the “terraform fmt” command as part of the pre-commit hook, it will make sure that all my terraform files are formatted in one way. So people that will review my Pull Requests, can not write any comment on formatting issues. 🙂

Before I end this blog post, I want to say one last things that really helps me with pre-commit hooks. I really like writing documentation in Asciidoctor format, i personally thinks it is a bit better that any other “language” and I don’t want to start a flamewar, but just writing on top of the document “:toc: left” and I have my Table of Content on the left side of the page. With Markdown, you’ll have to manually write one and keep the Table of Content up 2 date. Or, you install a tool with this command:

pip install md-toc

Then you can make use of the following line in your markdown file:

<!--TOC-->

And make sure that an id with “markdown-toc” is added to the “.pre-commit-hooks.yaml” file like shown in the beginning of this blog post and you are done! (small note is that you need to commit your changes to see that it is generated :))

“I can now see the benefits of using pre-commit hooks and will use them as well!“

– you?

I hope so? I hope I showed you how awesome pre-commit hooks are that they help you write better code. Not only that, it also helps the reviewers to focus on a PR that matters instead of looking for the nitpicky things. And with this blog post I only mentioned linting and formatting scripts, but you basically can do anything with a pre-commit hook. One small tip with using pre-commit hooks, don’t go wild on it. My commits will take some seconds to complete, but you can even run unit tests as well but that will result in longer duration of doing a commit. And when you do a “git commit” you basically are waiting on it to finish correctly, so don’t add lots and lots of tests because that will increase the duration of a commit.

But if you do have any questions and or remarks please let me know and I will happily help you.

In this post I will describe how to create your own pre-commit hook.

May the commits be with you!

The ethics of Pull Requests, improving the Pull Requests process

/ Werner Dijkerman / 3 Comments

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”

/ Werner Dijkerman / 4 Comments

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”

/ Werner Dijkerman / 3 Comments

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.