The Contentious Art of Pull Requests
Tags: git, codequality, standards, programming
Published: 2020-04-15 05:24:32

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.
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/9uhyv5iz8goy1ht4ozns.jpg)
## 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.
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/fbl9x3252njw34w6vb8w.jpg)
## 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.  
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/lpw38b4zb4tv84p52bce.jpg)
## 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.  
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/7c1fg6wyf34chv7o9ob5.jpg)
## 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. 
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/fppoeql0decyec9rt4st.jpg)
## 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.
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/0qqy9118nj3mcgqx2xpd.jpg)
## 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.
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/ys4jqwdopybi0m00j4pr.jpeg)
## 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.
<br/>

![Alt Text](https://dev-to-uploads.s3.amazonaws.com/i/0ze556x4jy3042z2e8da.jpg)
## 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.

1. 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.<br/><br/>

1. 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.<br/><br/>

1. _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_.<br/><br/>

1. 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_???