Dear GitLab: Please Remove Pull Request Comments!
PR Reviews Killed The Code Review Star
This article is about code reviews. Specifically, how much they’ve changed over the last decade, and more importantly, which aspects in particular because certainly some aspects have become better while others have gotten worse.
First things first: Are you a software developer? Then please before reading this article help us by taking two minutes to fill in this survey, which served as the basis for the article itself.
A few weeks ago, in a group of Agile Coaches we talked about code reviews. To be precise, review of pull requests. We found that in some teams there are people who output a lot of code changes, while others spend a lot of time reviewing those changes. Sometimes reviewing and providing feedback on these changes takes longer than actually producing them, which creates a situation where some team members do review code changes (asynchronously!) while more code changes are being done at the same time, increasing the backlog. So, we dwelled a little bit in nostalgia, reminiscing about the time when code reviews had only been face-to-face conversation in front of one computer.
Instead of speculating and writing a catchy post full of buzzwords and questions, we backed it up with (some!) data. (Last reminder about the survey.)
5/14 do up to five code reviews per week (less than 1 per day)
4/14 do five to ten code reviews per week
2/14 do ten to twenty
3/14 do more than twenty code reviews per week
If the [code change is perfect], 5/14 just accept a code change or pull the request without any comment or remark. 9/14 leave at least a nice comment.
In the case of [code change has minor deficits], fortunately nobody just accepts it without a word of feedback. Everybody leaves at least a comment regarding those deficits.
Once a code has real deficits — regardless of its urgency — ‘accepting’ the change drops to 1/14, and even in that instance it’s being followed up on with a personal conversation.
[Code change has major deficits] -> 8/14 Video call or talk to the person directly. But five of them still write elaborate feedback.
There are pro’s and con’s to writing elaborate feedback as well: On the plus side, it’s trackable — there is (literally) a record of the things that need improvement. On the negative side, this is what makes it so inefficient. The person writing it thinks long and hard about how to write it so:
- The essential points come across
- While at the same time not offending the person who submitted the change
The sad thing though is that no matter how much effort we put into writing things — there will never be any guarantee the person reading it won’t misunderstand. This is where speaking to someone directly is a lot more beneficial. At least we are more likely to pick up on any misunderstandings and clarify them instantly, before they can deeply and lastingly hurt the relationship.
[Code change has deficits, but is urgent] -> 7/14 Video call or talk to the person directly
[Code change has deficits, but is not urgent] -> only 3/14 Video call or talk to the person directly, with most of the previous cases defaulting to ‘leave elaborate comment’.
Interesting, don’t you think?
If it’s not urgent, we fall back to the less personal, more cumbersome way of communicating.
Which is the main point of this article: when things are urgent — we are effective and efficient, hashing things out in face-to-face conversations. While they require some setup effort (‘going somewhere’ or setting up a call), the advantages are immense as they’re faster, less prone to misunderstandings and shorten the feedback cycle to make it instant.
So why do we only do this when things are urgent? Given that ‘urgency’ is the exception, why do we choose the better way only in exceptional situations? I personally believe it stems from a premise to ‘not disturb the other person’. Which again is ruinous empathy (see Radical Candor). We are trying to be nice, while making our life and the other person’s life harder in the long run.
Are code reviews evolving or devolving? Are we being nostalgic?
Back to our initial conversation: From an Agile Coach perspective (most of us had worked as developers before) code reviews feel like they’re devolving. Back in the day, we felt each code review was a personal conversation which helped build lasting relationships between people. These days however, we’re mostly asynchronously reading code, alone with our thoughts and questions around that code.
So what did the survey say about that?
Changes in code reviews over the years
I gave each possible answer a weighting. Slightly more or less got +5 or -5 respectively. Much more or less got +10/-10 and ‘same’ got a neutral 0.
Here are the results for each question asked:
- 🡩 80: Number of single commits to be reviewed
- 🡩 75: Asynchronous feedback
- 🡭 40: Time you take for all code reviews per week
- 🡭 20: Acceptance of ‘cosmetic flaws’
- 🡪 5: Time you take for single code review
- 🡮 20: Face-to-face conversation around the change
The number of single commits and the amount of asynchronous feedback have soared, and with it the overall time spent on code reviews. Unfortunately while we spend more time on code reviews, the time we spend together has decreased.
Unsurprisingly the acceptance of ‘cosmetic’ flaws has also increased. No wonder as very few code reviewers want to be nitpickers around these, investing substantial work-time into writing down how to make micro improvements. In a face-to-face conversation this would just be a byline. Easy. Little emphasis. But still voiced.
The bottom line is while of course we (Agile Coaches) are being nostalgic, we think back to a time when code reviews were yet another point of human interaction in the process of software development. While technology is evolving, allowing us to make more frequent, much smaller code changes, have them covered by auto-tests, increasing our confidence in those changes, it seems like collaboration is devolving. Personal encounters that make us grow as a team while at the same time grow as individuals are getting less frequent. So it seems that nostalgia is not out of place here…
What can we do about it? What can GitLab do about it?
There are several things that could be done about it. My provocative appeal in the title of this post for GitLab (or Github respectively) to remove pull request comments entirely might well be on the extreme end of the scale, it’s not realistic for global companies where PR’s are being reviewed asynchronously and across continents.
Seriously though, how about a setting? So companies can configure these things themselves? We’re a small 20 developer company in one of the world’s start-up hubs? Deactivate PR comments — have people speak to one another. (Yet nothing planned on GitLab’s side. I don’t find anything about this on Githubs public roadmap either.)
We’re global already? Fine, have your asynchronous reviews. You need it.
Conclusion
As long as code will be written as a collaborative exercise there will most likely be code reviews. We need to remember that these are places for collaboration. When people collaborate there should be personal conversations. Tools can facilitate these conversations, but they can never replace them without taking away quintessential human factors.
Interesting follow up topics:
- How well are the why’s behind pull requests introduced? Has this changed as well? To what extent do people state the intention of the change?
- Has team-building become harder without this close interaction between developers? How do we even measure this?
Disclaimer §1: While this article was written and edited, additional people have participated in the survey so some numbers here refer to different points in time. This doesn’t spoil the basic statements within the article, since the distributions were fairly similar. When you view live results you will see different values yet again.
Disclaimer §2: Early feedback confirms the controversy of this topic especially in a time where asynchronous, remote work gets more popular and is often even required. One important distinction to highlight is: The suggestion is and was never to remove commit messages — this way code changes would go undocumented. It’s just about the follow up conversation.
