Don't Leave Comments on Pull Requests - Leave Pictures

A picture says a thousand words, right? Whenever you're reviewing pull requests (or reviewing code in any other context), you may find yourself writing copious notes, carrying on ad hoc conversations, and even doing "social media" kinda stuff - like +1-ing something, or "like"-ing it, or slapping emojis all over it.

But I had a little epiphany this week: Why am I burning all this time trying to type out coding instructions, and explaining application flow, and suggesting alternative methods? The fact is that my oh-so-handsome mug is, quite often, perfectly capable of communicating everything that I'm thinking on a given PR. So why not just slap the appropriate picture on it... and be done?

I know. You're probably feeling pretty skeptical about that idea. You may even think that I've finally reached the point where I need to find a new career, hang up the keyboard, and abandon my Dev.to ramblings. But you're only thinking that because you haven't yet seen the pictures that I'm gonna start slapping on PRs.

Observe... and be awed!

The Pencil-Whipped Approval

Ever had someone ask you to "review" their PR - but then it becomes immediately obvious that they don't want you to review it - they just want you to slap an approval on it? In fact, they may even get kinda snippy if you start adding comments to it. They don't want your input. They just want someone to stamp an approval on it so they can get it merged. In this scenario, Ima drop this pic on the PR.

NOTE: I didn't say that I'm gonna approve it. I'm just gonna put this pic on it. When they come back and ask me why I haven't approved it yet, I'm just gonna ask them, "Dooood - did you not see the righteous thumbs-up that I gave it???"

You Ain't As Stealthy As You Think

Ever seen a PR containing code that you know the dev was hoping to just "sneak by"? Could be something that skirts company policy. Could be something that is questionable from a performance or a security perspective. Could even be something that's been consciously obfuscated. This pic tells the dev that I know damn well what they're doing.

NOTE: This doesn't necessarily mean that I won't approve the PR. Heck... we all have those <ahem>moments</ahem> when we feel that there's just something that needs to be merged - for any myriad of reasons. Some of those reasons can even be borderline "legit". But this pic makes it clear that your effery has not gone unnoticed by me. And as long as you know, that I know, then we're all good.

Sooooo Much "Nope"

Look - declining a PR is something that I almost never do. I will almost always try to put constructive comments on the still-open PR. Or start a real-time convo with you to address some of my concerns. Or even ignore the PR. So if I go so far as to reject a PR, it probably means that you've done something (or are continuing to do something) in the code that's really starting to tick me off. In those oh-so-rare scenarios, a plain ol' rejection just ain't gonna do. This pic will certainly do a much better job of voicing my true feelings.

The Supa-Nope

Have you ever pointed out a problem on a PR, then the coder withdraws the PR, then at some point in the future, they re-submit the PR with the exact same code in it??? I've even seen the PR re-submitted at a time when they assumed I was out of the office and not paying attention (for which case, I need to come up with a pic that has three thumbs down).

A Conspicuous Lack of Back-Scratching

My PR's been sitting there for a week-and-a-half. You haven't approved it. You haven't commented on it. I've even reminded you that I need a review on it - but you still haven't touched it.

But now... you got an urgent PR and you're hyper-eager for me to jump in and approve it?? Yeah... sure.

You, Sir, Are One Clever MF'er

Maybe you think that all my PR responses are negative, but nothing could be further from the truth. Occasionally, your colleague submits something that's legitimately brilliant. At times like those, it seems a shame to do nothing more than slap an "Approve" on it.

Be Gone!

There are a few things that will make me end a code review immediately. Like, if your new JavaScript code has a var in it - any var in it - the review's done. I ain't even bothering to read the rest of it. Get this ish off my screen.

Just Here For The Comments

Have you ever pulled up a PR and it already has some epic thread on it that looks like a refugee from 8chan? Arguments. Dogmatic declarations. Someone quoting "company policy" (usually the same douchebag who can't be bothered to follow "company policy" in his own commits). Someone else arguing about the performance benefits of a for loop versus .forEach().

I usually won't touch those PRs with a 10-foot pole. Once the battle lines are drawn, my only "role" is to sit back and enjoy the ride.

The Sanskrit Whisperer

There are 1,000 LoC in the PR - and half of them consist of complicated regular expressions. The coder absolutely adores Lambda expressions - only because it allows him to write an entire component on a single line. Every variable name is an abbreviation - e.g., getUser() becomes gtUsr().

Let Us Never Speak Of This Again

You're gonna delete that PR. I'm gonna pretend that I never saw it. And there's no reason why either of us ever again needs to think about the ish that you just tried to commit. This is usually reserved for those times when someone's committed a chunk of code with egregious security flaws.

NOTE: This is truly a speaks-for-itself image. I have stared into the void (your code) and it has stared into me. And we are both shaken by the experience. All I should have to do is slap this image on the PR. If this is my reaction, and you still need me to point out what you did in your code that sparked this reaction, then you either need to be gone, or I need to be looking for another gig.

Oh, Really???

Have you ever known someone who tries to use future commits to "win" some kinda debate that you were previously having? Like, he's convinced that every string comparison should be done with a RegEx. So in his next PR, he converts all of the existing string comparisons in the codebase to regular expressions - even though that has nothing to do with the putative goal of his commit.

The Failed Experiment

I totally see what you were trying to do here. Heck, we may have even discussed this approach before you committed the code. And while we were talking about it, in theory, it sounded like a potentially-viable approach.

But now... Now that I can actually see the implementation? Yeah, umm... no. This gets pretty ugly in practice. And we gotta think of some kinda better approach.

The 50/50

Look... The code's not wrong. But it's definitely not right. I doubt I'm gonna be rejecting this PR. But I don't know if I have the intestinal fortitude to approve it either. It's, well... it's a start. There's some good stuff here. But mannn... it still needs some TLC.

The Haircut

This really ain't a bad commit. Seriously. There's some good stuff in it. But I'm prettttty certain that this 86-line function could do the exact same thing in, ohh... 10 lines or so.

The Big Baller

Ima be all over this PR. I won't do anything with it that's of use - to you, nor to anyone else. But I'm definitely gonna be all over it.

I'm gonna add comments, consisting of nothing but emojis, that bear no logical connection to the code where the comment is posted. I'm gonna reply to other commenters on the PR. I'm gonna drop comments about a cool new package I've been looking into - that has nothing to do with your commit.

NOTE: What I absolutely will not do, at any time, is approve the PR. Or reject it. Or make any direct reference to the actual code in your commit.

The Inspector

Do you remember the last PR that I submitted? I'm sure you do, because you gave me 37 suggested changes - for a commit that only contained 20 LoC. You quibbled with my naming conventions, suggesting that I convert getUser() to getMember(), only to ask me to change it back to getUser() when you realized that user is consistent with the rest of the codebase. You had me change my arrow-syntax functions into old-style function declarations, because you just don't like the arrow syntax. You insisted that the state values be drawn out into their own reducers, in their own separate files. You asked me to convert style={{marginTop: 2}} into its own standalone CSS class.

Consider this pic to be your fair warning. If you truly want me to continue with this review, I absolutely will. In fact, I may not do anything else but this review today. Cuz Ima pour through every single line with the petty, spiteful, immature spirit of vengeance.

You Da Man!

Have you ever reviewed someone's code when you realize that they've rewritten something that you originally wrote - and it's immediately obvious that they did a much better job. When that happens (and it happens quite frequently), Ima slap this pic on the PR.

The Histrionic Avenger

This pic isn't for any particular observation on the code itself. Rather, this is my response for anyone who can't help but launch into histrionics whenever they're reviewing your code.

Have you ever worked with a guy who can't just comment on your code, but instead has to act as though every minor "mistake" is a massive knock against your character?

"OMG, everyone! Check this out!! Adam forgot to add a semicolon at the end of this line! Who taught this noob how to get on the interwebs, amiright???"

For guys like that, Ima slap this pic on every piece of the code that requires a comment. I don't care how minor the observation may be. I'm gonna treat every iota of feedback as though it's The Worst Code I've Ever Seen.

Conclusion

Now that you've read this article, I'm pretty sure we can all agree that plain ol' comments just have no place anymore in a code review. Typing your feedback is just soooooo... six-months-ago. As long as you have enough unique expressions, you'll never need to type anything again!

Comments

[Vesa Piittinen]

The best PR is GDPR.

Have a cookie.

[Adam Nathaniel Davis]

There ya go!!! 😉

[Thorsten Hirsch]

I'm not good at this.

[myleftshoe]

Like the author said - a picture tells a thousand words!

[Ben Halpern]

omg

[Davide de Paolis]

Absolutely golden. I have sooo many of these facial expressions while reviewing... But then waste time trying to articulate them in polite comments Should really switch to images

[Jason]

Admit it, you just wanted to test out the camera on you new phone?!

I see what you did there!

:D

[Adam Nathaniel Davis]

You're not that far off! I've been doing a new YouTube channel and I have loads of stupid footage of myself taken with my new Canon M50 as I test lighting / lens / etc.

[myleftshoe]

Thanks for not using emojis

[DynamicSquid]

This is golden

[Bhupesh Varshney 👾]

I love this
Also how about we leave memes as well ;)

Bhupesh-V / memer-action

A GitHub Action for Programmer Memes xD

Memer Action

✨ Demo

❓ Usage

Example workflow

• You can use the following workflow as it is, just copy/paste in a file named greetings.yml inside your workflows folder.
• The reply action is performed by create-or-update-comment

name: Memer Workflow
on: [pull_request]
jobs
  greeting:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@master
      - name: Run Memer Action
        id: memer

        uses: Bhupesh-V/memer-action@master
        with:
          filter: "new"

      - name: Check Outputs
        run: |
          echo "${{ steps.memer.outputs.meme }}"
          echo "${{ steps.memer.outputs.title }}"
          echo "${{ steps.memer.outputs.source }}"

      - name: Create comment
        uses: peter-evans/create-or-update-comment@v1.3.0
        id: couc
        with:
          issue-number: ${{ github.event.number }}
          body: |
            🎉🎉 Thanks for opening this PR/Issue 🤗
            Please wait while the maintainer(s) review it

            Meanwhile have a look at this 😝 :

            > **${{ steps.memer.outputs.title }}**
            ![meme](${{ steps.memer.outputs.meme }})
            <sub>ℹ️ <a href="${{ steps.memer.outputs.source }}">Source</a> [

…

View on GitHub