My 3 ways of reviewing pull requests

Remco Gubbels — Sat, 29 May 2021 12:17:49 +0000

In my day to day life as a programmer, one of the main things I run into are pull requests. Either I made them myself and am waiting for a review, or I am asked to review a pull request from a colleague.

A pull request is a method of working within the coding community to get your peers to review, and ultimately approve your code contributions and merge them into the main branch.

In this blog post I want to share my three ways of reviewing pull requests and some general remarks that apply to all three approaches.

General remarks

Don't be afraid to ask questions if something is unclear.
Pull requests (PR's) should have a short life. PR's that are open for too long will go out of sync in an active code base and create more and more work.
Repeating yourself in a review, ie. commenting the same thing on a repeating factor, can be seen as negative. Try to keep it to one remark that includes the other cases.
All automated tests should pass in any situation and need to be added if applicable.

Open source reviewing

A few years ago I was attending a presentation by Pieter Hintjens regarding managing Open Source communities. One of the things that I remember from this talk was his view on how to handle pull requests within such a community and what 'joy' it sparks for a contributor to get their pull requests merged quickly. His way of reviewing was to not really review it, just merge and see later if it works. If it breaks revert it or fix it yourself.

Back then, my initial reaction was this is never going to work in the projects I work on. However over the years I have started to develop my own way of reviewing pull requests, and I have to say that his approach does work within some projects I have worked on, Open Source and internal ones.

Intense reviewing

This is the most 'nit-picky' way of reviewing a pull request. Intense reviewing is used within our team at work to uphold strict styling, test coverage and a way of writing code in order to keep our code base synced across multiple repositories.
During this kind of reviewing process you go file by file and check if there are (CI system should indicate) any issues regarding leftover console.logs, missing semicolons and more importantly, stuff that is hard to detect for 'linting' tooling, typo's, missing comments if necessary etc.

These kinds of issues of course do not need a long comment. In our team we sometimes just resort into using either an emoji or just a ?. Make sure that you only do this if you know it is not perceived as a 'bad comment' else it can have a negative effect.

The next step of this review process is to really understand why certain decisions are made, especially if you think you would have solved this issue with solution A, but solution B was chosen.
Here are two ways of commenting on this kind of topic:
The telling way: 'I think solution A is way better' or (in my opinion) the better, questioning way: 'Why did you choose solution B here?'.
If you are not satisfied with the reasoning, you can go in to a more detailed questioning, e.g. 'Do you think solution A would be better?' Keeping it as a question should spark a discussion and hopefully you both learn something out of it, in the end it does not matter which solution is chosen.

The aim of a review process like this is to completely understand the reasoning of how and why this code change has been done, especially to make sure their are no broken windows in the code base you and your team are responsible for.

Another benefit when using a review style like this with newer developers is that they have a 'safety net' while coding, which means that they should feel like they can try things, won't break anything and also learn and improve themselves.

Quality driven reviewing

Quality driven reviewing is a way of PR reviewing if something is currently required, but in the long run is not going to be there. e.g. something that is required for a short migration period, this is the middle ground of the three types of reviewing.
Sometimes your team needs to release a feature that is temporary (yes I know, most things temporary in a code base are still there four years later) or under a very tight deadline.

Deadlines or temporary solutions are never an argument for bad code. The intense way of reviewing normally takes a bit more time and sometimes could be a waste at that moment due to the duration the code will live in the code base, or take too long for a deadline that needs to be met. During this way of reviewing you check if the code is correct and does not break anything else. Even if you think that solution A should be replaced with solution B as it's 'better' or 'more in style' this can be skipped unless it is necessary for performance/stability.

Just to be sure, only pick this method of reviewing if you cannot do the intense way of reviewing, it is doing the bare minimum, but sometimes that can be enough.

Do you have other ways of reviewing or do you have any remarks regarding the ones I have mentioned here? Let me know!
Also do you sometimes do the fourth option of reviewing, namely even if you know it's broken, not in style etc, just merge it and fix it afterwards as you're 'done' with commenting?

The reason I will start blogging

Remco Gubbels — Sat, 13 Apr 2019 00:53:23 +0000

The question that has been popping up in my head for about the last 2 to 3 years, should I start blogging about my learnings when it comes to being a developer or not?

Why should I do it?

If you browse around the different blog sites, from Medium to Dev.to obviously the answer you find on those pages will be yes, blogging does wonders for you:

Improves your writing skills
Improves your critical thinking
Organizes your thoughts and learn
Promote yourself to a wide audience

Why didn't I do it before?

In my head I also answered this question with yes, I should, many times. But I never followed on that answer to actually post and publish something. There are some out-dates pages on different file storage systems, from work related to personal posts etc, but nothing ever ended up on a blog site. Besides that, another question was always asked: where should I put my blogs? Should I write my own blogsite? which always followed with a long silence of indecision and sometimes creation of a starter app.

From my point of view, it always comes back to the same well-known scenario, the problem with finishing things. I start enthusiastically with writing the outlines of the blog posts, just as I'm doing at this moment (this of course will be somewhat invalid if you're reading this...) and leaving it for another day to finish the post and read it over, but this other day never occurs or way later in the future when the post is outdated and needs to be tweaked in order to be valuable.

Why will I do it this time?

At my job we have to come up with goals for each year how we want to develop ourselves and set targets that are obtainable in that year. As I mentioned before writing a blog has been in my mind for the last few years, it also has always been on the 'backlog' of my goals or as an underlying part of another goal. It always ended up not being picked for other goals. This year I'm going to change that and see if by adding an extra motivation (better end of the year assessment), I will go through with it and actually pick up blogging in order to see if the above statements about improving myself are true.

What will I blog about?

I find it always hard to pick a subject where you can blog about that might be interesting to people to read and find actually useful, so I attached it to another goal that I have for this year. We just started adopting Ansible at our work and it's a new technology for a lot of people so I decided to follow an online course to get to know it. After gathering this knowledge, I could have decided to use this for myself and share the link of the course with other people, but this way I wouldn't improve myself in the area that I think I need the biggest improvement, explaining things simple and understandable to other people. That also goes hand in hand with the quote: "If you can't explain it simply, you don't understand it well enough.". Which accordingly to this post is falsely noted as an Einstein quote and was never said by him, however I still like it.

So, I've ended up challenging myself by creating an internal workshop for colleagues at work, but next to this I'll be creating a series of blogposts that will be aligned with the different steps in the workshop and hopefully help people in their search for Ansible knowledge. Besides the workshop steps I'll be doing a high-over post on the technology stack that I'll be using in the workshop, which be based on a Lit front-end with a NodeJS API as backend (still haven't decided on which server base to use) and the end result will be a site with build in analytics and database, which of course has been done multiple times already.

So here I am, basically answering the question I have been asking myself for the last few years, should and will I start blogging, yes, I'll give it a shot and I'll see where it will take me in the coming months.

DEV Community: Remco Gubbels