Reviewing Pull Requests Like a Human

You ever stare at a pull request (PR) with 40 open comments, all of which require resolution or response in order to merge your work? I have. I learned loads from those, we would get into great discussions documented right in your project. But it’s a double-edge sword, you can learn a lot from these experiences and at the same time it can also suck the life out of you leaving you just a desiccated hunk of VSCode shortcuts and a mild caffeine addiction.

It doesn’t have to be that way. It can actually get worse, pull request reviews can become a politic dumping ground and a toxic element of broken engineering cultures and horrible teams.

You deserve better.

You invested in the work you did and your reviewer invested energy and attention into reviewing it. It’s easy to forget that, your reviewer is an impediment to your progress, and you’re a distraction from whatever they were working on. You both are worthy of respect and kindness.

So I developed a system for PRs I like to float when I’m working with teams. I call it: Ranked PRs.

These are kinda my rules for Ranked PRs:

But the manner for doing all of this is pretty blunt: If you’re leaving a comment on someone else’s work it should be qualified or at the very least categorized.

In practice, I end up prefixing each comment:

Emojis for some teams might be neater:

This accomplishes two big things:

  1. You get a focused scope as the reviewer to work in. You can prevent a lot of hassle from knowing you have a finite way of responding; fewer rabbit trails to go down and clearer targets to hit.
  2. You give the author of the PR a way to navigate your feedback. For instance, if they’ve got 4 REQUIRED comments, they can resolve them quickly and focus on bigger pieces of feedback.

It ends up looking like this:

A comment on a code review with a NIT flag A comment on a code review with a REQUIRED flag A comment on a code review with a PRAISE flag

Making praise and ignorable comments a key part of this setup makes it easy to call out growth and identify what issues you should let go of in time. For instance do I actually care if someone is casting something to a type? Maybe I do, maybe I don’t, but calling it out gives them an opportunity to explain or ignore you and you can be okay with either. Should another engineer get some praise for making a cleaner abstraction for that one thing that bugs everyone? Yes, yes they should.

However, I have found some caveats (YMMV):

  1. In teams that are growing I’ve found this method for commenting works effectively, especially when you have a mix of junior and senior engineers.
  2. If you present this as a prescriptive solution it will fail, it’s usually better floated as a discussion topic or an experiment.
  3. If you haven’t automated formatting and code linting rules (think SwiftLint, Prettier, ESLint, etc), you’re in for a bad time. It’s not worth the collective energy to keep calling these things out and usually ends up looking like dog-piling on someone. Like just set it up and if someone has a real issue with it, have a conversation and be flexible. (My experience has been bad managers don’t like these tools but they don’t have to write the code so 🤷.)
  4. If you have someone new to a team or new to coding and it’s their first PR, setting expectations of “Hey, 40 comments is totally normal, not an indicator of anything wrong” can be so helpful.

There are some tools that can help make working like this a little bit easier:

A legend for PR comments

Ultimately GIFs and emojis cannot fix a broken engineering culture, but a little bit of levity and lowering the stakes of giving feedback fosters a more giving group of engineers.