This is one of those "we shouldn't need a blog post about this" topics. But sadly... I really feel that we need a blog post about this. I'm talking about the modern practices surrounding code reviews and pull requests.
A Little Context
I'm old enough to remember code reviews that were done with printouts, or with projectors, or (good gawd) on whiteboards. Not only were these reviews difficult to manage tactically, but they were also highly-impractical with regard to true code quality. Because by the time someone conducted a "formal" code review, it was frequently the case that the code was already, mostly done.
So by the time that the rest of the team was laying eyes on it, we could possibly provide some helpful suggestions for last-minute cleanups. But it was nearly impossible to correct wholesale alterations that should've been implemented in the new module/feature/etc.
Even with the early generation of source control tools (e.g., Visual Source Safe or SVN), it was difficult to really get eyes on the work being done by the rest of the team. And when you did manage to spy their code, it was too-often done after it had already been deployed to production.
It's no exaggeration to say that git represents a quantum leap in this area. Nowadays, you don't have to use git. But most of us do. And I really think that git provided one of the best paradigms for group review of code on an ongoing basis. Even if you're using some other source control tool, the pull/merge request process is probably closely analogous to what we currently have in git.
Code Collaboration Comes Of Age
If you're working in a collaborative-coding environment, the pull request is one of the most powerful tools that you have to ensure that everyone sees (or at least, has a chance to see) what everyone else is submitting. Ideally, some (or all) of your team members are perusing most (or all) of the code before it ever gets merged into a parent branch.
This is powerful. This is good. There's no way I'd want to return to an age when we didn't have such convenient realtime tools to view-and-comment on everyone else's code.
But like all good things, even pull requests have... downsides.
A Forum For Snark And Pedantry
Developers, as a general class of people, are known for being a bit... snarky. If we were amazing "people persons", we wouldn't have chosen a career field where the vast majority of our time is spent staring at screens. With headphones blocking out all ambient noise (and any "real" interaction with other people).
Developers can also be very... opinionated. (SHOCKING!!!) Non-developers don't even understand the Holy Raging Fire that is spawned by something as "trivial" as tabs-vs-spaces. Or leading-commas vs trailing-commas. Or classes-vs-functions. But developers already know these debates well. And they're firmly entrenched in one side or the other. And they're likely to shout you down if you dare submit code that doesn't appeal to their inner preferences.
As much as I embrace the pull request process, I must also admit that I've seen it go horribly wrong. Too many times. When you invite another developer to look at your code, you can almost hear them cracking their knuckles, taking a deep breath, and girding themselves for a Snark Battle.
So, even though I really wish that this post were completely pointless, I've come to realize that I've accumulated my own internal series of "Pull Request Rules". And I really hope that, somewhere out there, maybe a few of you will read these - and take at least some of them to heart.
Rule #1: Linting Is Not A "Nice To Have"
My third post on this site was all about ESLint. (You can read it here: https://dev.to/bytebodger/why-i-hate-eslint-and-how-i-learned-to-love-it-40bh) Specifically, I admitted that I originally hated linting tools. They were like some annoying nanny looking over my shoulder, telling me that my chosen style of coding was somehow "wrong".
Today, I love linting tools. I didn't "come around" to linters because I enjoy (or even agree) with all of the pedantic rules they enforce. I came to rely on them as a critical tool because of the pull request process.
Rule #1: ALL nitpicky details about code styling should be defined/enforced in the linter. If the code you've written doesn't satisfy someone's internal idea of "good coding style", but it passes the linter, the time to argue over this is not during the pull request process.
You see, a good linter should be the first-and-primary defense against any of the Code Nazis on your team who want to hold up your pull request over some pedantic detail of styling.
Does your team want spaces-over-tabs? Then it'd better be enforced in the linter.
Does your team want the opening curly brace on its own new line? Then it'd better be enforced in the linter.
Does your team want case statements indented inside the switch block? Then it'd better be enforced in the linter.
If your team is still trying to "enforce" code-styling standards by policy, and can't be bothered to take the time to implement a shared linter, then... welcome to 2002. Your Nickelback concert t-shirt is waiting patiently for you.
I recently worked on a team that thought they were wayyyyy too busy to implement any linting tools. But they still had their own internal hodgepodge of "coding standards" that they felt compelled to enforce. It didn't matter that their so-called standards weren't even consistent in their own codebase. I'd code something up, submit my pull request, and then they'd hammer it with a slew of stylistic changes that they wanted me to make before it could be merged.
When it comes to stylistic decisions, the question should be dead-simple: Did it pass the linter? If it passed the linter (or if you couldn't be bothered to even implement a linter), then it's an extremely poor (and adversarial) practice to use the pull request process as a haphazard means to enforce your poorly-defined "standards".
In case I'm not being perfectly clear, if you feel compelled to litter someone's pull request with a series of dictates to change coding style, you're being a Pull Request Jerk.
Rule #2: Humility
Too many times, I've seen PR comments where the reviewer confidently proclaims that something is "wrong" or must be changed. Even in the egotistic world of application development, such inviolate proclamations are often flawed. And they rarely foster good esprit de cor amongst your teammates.
Rule #2: If you make a bold statement on someone's pull request, you'd damn-sure better be right. And even then... you probably shouldn't be making such bold statements on someone's pull request.
Have a little humility here. Even for The Most Senior Of Developers, it can be extremely challenging to simply look at snippets of code and truly understand everything that's happening there - or why specific coding choices were made. There's always at least a chance that the reviewer just doesn't fully grasp what's happening in the code.
Consider the difference between these two sample PR comments:
Are you sure that you want to use a regular expression here?
It seems this might be more efficient with a simple string comparison.
Vs.
The RegEx used here is inefficient. Change it to a simple string comparison.
The second statement is damn... confident. Arrogant, even. The second statement is more likely to spawn a defensive response from the person who submitted the code.
[NOTE: I'm not even getting into the question of whether-or-not you're right. In the example above, it's entirely possible that the use of RegEx is inefficient. It's also possible that the coder really should change the snippet to use a simple string comparison. But that's not the point.]
Couching your feedback in a little humility will usually get a far better response on the PR. It will foster greater respect between everyone who's reviewing the code (including, but not limited to, the original coder).
And no matter how confident you think you are, there's usually at least some chance that you're actually... wrong. Or, at the very least, there's always a possibility that the original coder had a cogent reason for writing the code the way that they did.
Questions foster discussion. Statements foster debate.
Even in those rare cases where I absolutely know I'm right, I rarely put an inviolate statement on the PR. For example, I've seen PRs where the code simply wouldn't compile. Did I code-shame them by stating that something was broken? No. I usually leave a comment more like this:
It doesn't look to me like the line above will compile. Am I missing something?
Often, in these scenarios, I know that I'm not missing something. But I've found that simply dropping the comment as a far-more-gentle question usually leads to a much greater sense of ongoing collaboration.
Rule #3: Bulldozing Comments
The whole point of a PR review is to encourage as much group participation in the process as possible. One way to completely undermine that participation is to ignore someone's comment altogether and simply merge the code anyway.
Rule #3: No pull request should ever be merged until all of the comments on the request have been answered.
[NOTE: I'm not saying that every comment/question has to be answered to your satisfaction. In a high-velocity environment, it can sometimes even be acceptable if someone answers your comment/question with something like, "Yeah - this needs to be refactored, but for now, we just need to get it merged so QA can start some of the testing process."]
There are few things more disconcerting than to spend careful time reviewing - and commenting - on someone's PR, only to see some hours later that it was merged without any change being made to the code, and without any answer being placed on your comment. When this happens, I refer to it as bulldozing comments.
Recently, I worked on a team where the tech lead bemoaned the fact that their PR process was "broken" and that most of the devs didn't really participate. After working there for mere weeks, it was painfully obvious why this was the case.
Unless you were one of his "chosen devs", you could carefully inspect/comment/question the request - and it would still get merged anyway. Without any response. And without any changes made to the code. It didn't take long for any new devs on the team to realize that it was a waste of time to bother reviewing anyone else's code. They either slapped a blind approval on it - or they just ignored the PR altogether.
Rule #4: Code Avalanches
The previous rules are all about the people who are reviewing the code. But the submitter also bears responsibility in an effective review process. One of the greatest sins committed by the coder is that of the Code Avalanche.
Rule #4: Pull requests should always be as small as possible.
Let's be honest. We've all been guilty of this on occasion. We're working, on our localhost, on The Next Great Feature. It involves dozens of new files. Or massive changes to existing files. It encompasses many thousands of lines of code. And for various reasons, we've been hoarding it in our own private branch that's never been pushed to anyplace where the other devs can see it.
When we're finally ready to get this behemoth deployed to some lower-level environment, we create The Pull Request From Hell. We add everyone who's ever touched the codebase as an approver. And then we wait anxiously for their rubber-stamp approval.
And let's be frank. When we commit this sin (and this code), we all know, deep down, exactly what we're doing. It's such a massive cognitive dump that no one else can possibly hope to read through everything we've written and provide any meaningful feedback. We're secretly hoping that someone will just be overwhelmed by all changes - and just... approve it.
Simply reading code is hard. It's a big ask to expect anyone to really grok the logic when it's not running in their local environment, but instead it's simply "printed" on the screen. This challenge becomes exponentially more difficult when you're asking someone to review thousands of lines of new/updated code.
When you dump a code avalanche on someone, it's possible that they might be able to spot small imperfections in your code. But it's ridiculous to think that they might be able to provide deeper feedback on whether your overall approach "works" - because they'll be challenged to even understand the broader goals of what your new code is doing.
Rule #5: Time Hostages
If you really care about the code-review process, then you should know that any meaningful review takes time. It shouldn't take days. And it should rarely take hours. But it definitely takes time. How many times have you seen an email/chat message like this?
Hey. I just submitted this pull request. Can someone please
review/approve it? I'm trying to make the automated noon deployment
to QA.
Then you look down at your watch... and it's 11:55 AM.
Rule #5: It is always the responsibility of the coder/submitter to ensure that the requested approvers have reasonable time to provide a good-faith review of the code.
This is another one of those "we've all been guilty sometimes" issues. I get that. But acknowledging our own complicity doesn't make the problem any better. If you're standing over my shoulder (virtually, or physically) begging for an approval, and my timeline for doing so is minutes, then congratulations. You are, officially, trying to turn me into a Time Hostage.
Of course, there are numerous exceptions to this. If I'm confident that the code I'm "approving" will never, in its current state, make it clear through to production - then, yeah... I might be happy to slap an approval on it. But if this code is truly on the fast-track to be deployed live, then I won't be having any of your desperate last-minute pleas.
Rule #6: Accountability
If there's any part of the PR process that's routinely "broken" - in sooooo many dev shops where I've worked - it's the willful lack of accountability. What do I mean by this??
Well, when you slap an approval on someone else's pull request, you absolutely should consider it to be your own personal endorsement of the code. In the rare cases where something truly "bad" makes its way to production, I've (unfortunately) seen the originating developer thrown right under the proverbial bus. But I've rarely seen anyone ask, "But who approved this code?"
Rule #6: Approvals on pull requests should never be taken for granted. They are a personal statement that at least one other developer reviewed your code - and they stand behind it.
[NOTE: I'm not saying that your "approval" means that you absolutely love the code - or what it's trying to accomplish. But I am saying that any approver should bear at least some culpability for any egregious errors that go unchecked.]
At the risk of lawyering this, I want to point out that there are key differences between "culpability" and "responsibility". If Joe managed to get some horrendous code into production, and it eventually brought down the entire site/app, who is ultimately responsible for that failure? Well, Joe, of course. I'm even OK with the idea that Joe might be disciplined - or, in the worst-case scenario, terminated. But if Bob approved that code, then he should also be held, at least to some degree, as culpable for the failure.
This is a really touchy subject in dev shops. On one hand, none of us wants to bear the brunt of the crappy code that Joe managed to get into production. On the other hand, if the approvers on the pull request are absolutely free of any consequences for their actions, then why should they care about giving any meaningful review to the code before they "approve" it??
I've actually seen this play out - positively and negatively - in live environments. I've been on teams where no one dared to ask, "But wait... who approved this code?" And on those teams, approvals were cheap.
I've also been on teams where approvals did indeed bear some degree of accountability. And you know what? On those teams, it's amazing how much more meaningful the approval process became. When people know that their approvals could, in theory, be scrutinized after-the-fact, those people will magically stop "rubber stamping" pull requests. Those people start paying careful attention to any commit upon which they've placed their endorsement.
If this sounds a bit like a "witch hunt", it's absolutely not meant to be. In the scenario I outlined above, the originator of the crappy code (Joe) should always be the one who bears the greatest consequences of his actions. But it's a dire mistake if Bob is not at least questioned about his approval on that crappy code.
I also want to make it clear that the responsibility I'm referring to should only be leveraged in the case of the most egregious mistakes. Bugs happen. I get that. And no one should be expected to have spied every possible bug in your code simply because they read through it in a pull request.
But if something was submitted (and, god forbid, deployed to production) that qualifies as a blatant error, the approver(s) on that PR should at least be called into question. If they're not, the quality of the entire team will suffer. Because the pull request approval process will have been rendered meaningless.
Rule #7: The Ban Hammer
If you truly believe that a pull request is wrong, then when do you actually go so far as to decline it?
Oh, man...
If you think that pull request accountability is a touchy subject, just wait until you get into all of the political landmines that come with declining them. For many devs, declining someone's pull request grates on a lot of raw nerves. It shouldn't. But let's be honest. It absolutely does.
So how do you navigate that minefield?
Rule #7: Declining a pull request is a last resort. But avoiding that last resort should never be an excuse for an approval.
First, let's acknowledge that there can sometimes be... feelings that are wrapped up in declining someone's PR. In a perfect world, none of us would be so emotionally involved in our code. But this is far from a perfect world. And developer personalities can be... touchy. Declining a pull request often feels, to the requester, like you've slapped an "F" on their work.
Second, let's take stock of all the options you truly have at your disposal.
As long as your team isn't inclined to Bulldoze Comments, the commenting process itself is often a solid alternative to outright-declining someone's PR. In the example I listed above, if you've put a comment like, "It doesn't look to me like the line above will compile. Am I missing something?" that should be enough to keep the code from getting merged until it's either "fixed", or you've received greater assurances that it actually does compile. So it's usually poor form to decline someone's PR, assuming that you've notified anyone else on the request that you have a problem with it - and that it should be addressed.
You aren't usually required to participate in the approval process. This can be problematic on smaller teams. But assuming that there are other requested approvers on the PR, it can be perfectly valid to just "sit this one out". I've literally had situations where someone came over and directly asked me to approve a PR, I looked at the code and thought it was horrendous, and I told them, point-blank, that I wouldn't approve the request, but I wasn't going to decline it either. In other words, I was basically telling them that I wouldn't personally put myself on the hook for approving it, but I wasn't going to stand in the way if someone else was comfortable doing so.
Deleting a pull request can be a much better option than declining one. For example, imagine that you've seen an outright compiling error in a pull request. You're certain that it simply won't run if it's deployed. But the requester isn't currently at his desk and you're afraid that, if you just sit back and do nothing, someone else might mistakenly approve the PR and it will be deployed to some other environment. In that case, I've often put a comment on the PR indicating my concern - and then I deleted it. Even though it's been deleted, the original submitter should still receive the email notification for the comment that you left on it before you deleted it. And if, for some reason, they don't receive notification of your comment, they'll probably just come over and ask, "Hey! Why did you delete my pull request???" This may feel like it's no different than declining the request. But in my experience, it's amazing how much more open devs are to finding that their request has been deleted, rather than being declined.
There is no requirement that all communication be encapsulated on the pull request. In our modern world, we sometimes forget this simple fact. We assume that, if Joe created some "super janky" code, we have no choice but to point it out on the pull request. But this can feel, to the submitter, like a form of "code shaming". Sometimes, we feel we must go so far as to decline the request. But there are many times when I've just walked over to Joe's desk, or hit him up on Slack, and said something like, "Hey... I saw your latest PR. But something looks really off to me. Is that really what you meant to submit??" In such scenarios, I've often found that this simple act of personal communication is sooooo much more effective than outright declining the PR. This can also be helpful if you're faced with a Code Avalanche. In those cases, I've sometimes pulled the developer over and said, "Walk me through this."
Conclusion
I'm fairly certain that I've left out some critical points. Heck, this might even spawn a Part 2 at some point.
This post might feel like it's full of "bold" (or even, arrogant) statements. There are many aspects of software development that I honestly feel do not lend themselves to hard-and-fast "rules". But as to the stuff that I've outlined in this article, I feel very strongly about it. It would take a lot to convince me that any of these rules are, in fact, flawed.
But... I've been wrong before (allegedly). What say you???
Top comments (8)
Currently I work on a project with someone that absolutely loves to bulldoze comments as per Rule #3.
It's infuriating to spend an hour to actually review PRs, give feedback and discuss questionable additions, only to see that 4 out of 5 reviewers get skipped and the PR merged.
I don't even know how to discuss this with the dev, without it sounding like an attack. Do you have any advice how to go about this?
Depending on your tolerance for confrontation, this scenario can indeed be tricky. I know how I'd deal with it, but most people aren't as comfortable as me when it comes to pissing people off.
The first question I have is: Are you simply leaving "comments"? Or are the comments entered after you've clicked "Start a review"? I'm asking because if you "Start a review", the comments will display as needing to be resolved. But if you haven't started a review, your comments are more like side notes.
Second, what are the PR settings for the repo? Specifically: how many approvers are required before it can be merged? And who has permissions to merge? If the PRs require more approvers, then it's less likely that it gets all the required approvals without your comments being resolved. Similarly, if there are only a few people who can merge, and those people are at least semi-professional, then it's far less likely that unresolved issues get merged. But if the policy is that anyone can merge once it's been approved, then it's far more likely that the dev can find someone to slap an approval on it - and then he can just merge it himself.
Obviously, changing the PR/approval/merge policies requires some buy-in from the rest of the team and it can lead to further headaches. For small teams, it can be impractical to require any more than a single approval. And locking down merge rights can be problematic if, say, only one person can merge - but that person's not in the office at the moment.
But these approaches are all tactical ways to control what is really a systemic issue in your team. Ultimately, the team's leadership (and, by extension, the entire team) shouldn't tolerate that crap. It's one thing if you put a single, small comment on a PR and it gets merged without resolution. That can happen anywhere - usually by mistake. It's another thing if your feedback is being completely disregarded.
If you're not comfortable talking to the dev about this directly, you should definitely be comfortable talking to the manager/lead about it in private. (And if you're not comfortable talking to the manager/lead in private, then you really need to question if you're in the "right place" for you.) Hopefully, the lead would understand your concerns and then communicate them back to the whole team as general policy (i.e., communicate them without having to call you out specifically).
If none of that works, then I ultimately believe that you need to protect your own sanity by simply withdrawing from that dev's reviews. In other words: Don't disapprove his PRs. Don't delete them. Don't comment on them. Don't do anything with them. If you know that your time spent reviewing them will be WASTED, then why continue to spend the time??
I understand that this approach is kinda passive-aggressive and may still lead to some kinda low-level "confrontation". At some point, Jerk Dev will say, "Hey, I submitted this PR. Can you please review it?" And you'll need to say something like, "No - my comments are always ignored on your PRs, so I can't afford to waste the time." But if the team/lead/management won't enforce a general policy of resolving comments, this may be your only real option.
First off, thank you for the detailed response!
I am usually a very direct person. This is very uncomfortable for others, but sometimes I do think to get my point across it's necessary to skip the sugarcoating. Though I have been told to "dial it back a little", which I am failing most of the time.
Currently, we are using bitbucket, which IMHO, is heavily lacking in that regard. It seems the moment you are assigned as an approver, the review has officially started, but can, at any point, be overwritten by the one that created the repo. In my case, that's Mr. Merge-Happy. It might also be some very weird repo settings, which I can't view as a contributor.
What might be important information is, that I explicitly marked the PR as "needs amendment", or whatever bitbuckets English translation for the yellow "not good enough yet" button actually is.
To me, it looks like this is the case at the moment.
Unfortunately, there is no real "lead" in this project, the higher-ups decided "let's change it up for once! You have a very flat hierarchy, you duke it out amongst yourselves." I'm not sure what else to expect but pure anarchy from this approach. If there is anyone we could consider "lead", it would probably be the dev in question. Which makes this a rather spicy topic for a talk with the manager.
I did have a talk with the dev, because it ticked me off so bad, the response was basically "if the PR is still open after 3 days, I'll just merge it. Rule of cool."
So, as much as I would like to make use of some of the great points you made, I think with that kind of attitude, pretty much every discussion would be a waste of time.
That leads me to the conclusion that the passive-aggressive approach might be the only valid way, if my monologue about "collaboration means working together, not against each other" shows no effect in the near future.
Thankfully, as an external contractor, I can just go on vacation for a few weeks and watch the mayhem unfold from the outside.
Your feedback is very much appreciated, I learned a lot from your response!
I approve your pull request. You can merge this post.
Great wisdom in minimal space, 2nd approval from me !!!
I love it, really great points. I think that where I've worked most people do some or most of these without directly thinking about them. But having them all outlined like that... This is the kind of thing I would have in writing and would officially assign as "company standards". Thank you.
Thank you! I'm glad you find value in them - and that's exactly why I felt compelled to write them up. Cheers!
The Emotions are Real! π