Thread by @TejasKumar_

Since my React thread went so well, here’s one filled with things I’ve learned about Code Review, after reviewing pull requests for the last few years.

A thread. 👇🏾

It’s all about the communication. If you work on a software team, being explicit can really help.

I’ve found that the code isn’t necessarily as important as its intent. Making the intent clear helps the team understand and potentially improve the overall product.

Here’s an example: you want to ignore a lint rule.

Not ideal:
// eslint-disable-line

Ideal:
// eslint-disable-line because (your reasons here)

I love that // comments continue to be ignored through the end of the line so you have all the room to talk to your team. 💞

Given that it’s about communication, I’ve often reviewed my own PRs first as soon as I’ve opened them and left comments around questions that I know my team will eventually have: sometimes comments in the code, other times on the PR itself depending on the context.

This helped

A little encouragement goes a long way. When reviewing code, if you’re in the same room, appreciate things out loud as you review (if it doesn’t distract others).

“That is a lovely mapReduce!”
“LOOK AT THAT TEST COVERAGE”
etc.

I’ve found this creates a culture of encouragement.

It’s not just others who hear you do this, _you hear yourself_ say these things, which bolsters your love for your team.

At least it does for me. I’m honored to be on the same team as my coworkers. Their work moves me every day.

Needless to say, positive textual comments in the reviews also help. Code review does not mean “point out only the problems.”

“Code review” for me is two-fold: it is not simply reviewing a diff, but also testing things.

We deploy every PR so we have a working “demo” where we can use the new product feature/bugfix.

This helps us know that not only does the code look good, it also works great.

For our open-source projects, we use @netlify’s pull request previews. They’re incredible and allow us to time-travel debug our product by just going to

…review-PR_NUMBER--project.netlify.com

This has been super valuable for us tracking down regressions. I highly recommend this.

Kudos to @fabien0102 for introducing this to our workflows. We benefit from PR previews more or less every day.

You’ll also want to take some appropriate measures “on the ground” before commits ever hit the cloud by formatting things for minimal diff-noise in pull requests.

A git precommit hook using @okonetchnikov’s great tool lint-staged along with @PrettierCode adds immense value.

Questions questions questions.

I’ve seen a lot of reviews with unwarranted “oh no this is wrong, do it this way” comments and not enough “why are you doing it this way?” or “are you aware of X?” comments

Often times, the author has more context and sound reasons for their code

I recently wrote in a review of:

Array.from({ length }).map()

“Are you aware that Array.from has a second argument which is a map function? It might help to make things more concise.”

Turns out, it was helpful. I would always err on the side of questions over comments.

Just as we pair program, we can also pair review. We often pair up between author and reviewer and discuss PRs briefly. It helps ship things fast, while understanding the code and thought process surrounding it more easily.

As a favor to your team, I’d suggest creating smaller PRs at a time to reduce their mental requirement for reviewing that will in turn get your changes merged faster.

From experience, a hundred smaller, iterative PRs help everyone more than a single giant pull request.

Why?

Big PR:
- open, get overwhelmed
- start review
- review 1/3 of the way
- oh it’s time for a meeting
- come back
- ah I hope I didn’t miss something
- start over
- repeat
- eventually finish review
- start review again just to be sure

Small PRs:
- nice
- approve
- merge

I’ve seen regressions from larger PRs where the code just simply could not be reviewed as thoroughly because we’re human. I don’t think that’s a burden we ought to place on our peers.

With smaller PRs, everybody wins:
- ship faster
- less mental overhead
- more iterations

👌🏾

If for whatever reasons I cannot avoid a large PR, I try to have it composed of small, cherry-pickable commits because:

1) It does not block others
2) It does not require duplicate work: just cherry pick my commit
3) It is easier to review by going through each (smaller) commit

D A N G E R

Danger is a great tool that’s like a linter but for PRs. We use it, I think @apollographql use it, and a whole bunch of others.

It checks PRs and if they don’t satisfy your constraints, it fails a CI-style check and posts a comment letting the author know how to fix

It’s quite cool how much it can do. It runs on most cloud git services (@github, @gitlab, @bitbucket, etc.) and executes JS/TS so you can do quite a bit.

Here’s a link: github.com/danger/danger-…

We all get busy. Sometimes we need to remind our teammates about our review requests.

I built a @slackhq integration that we use at work that pings the GitHub reviewer requested on Slack when a PR passes all CI checks and is ready for a review.

This ensures timely reviews 😍

Another helpful Slack Integration that I wrote is a serverless function on @zeithq invoked on a regular interval that pings our channel with open PRs every morning, @-mentioning the user on Slack requested for a review.

It’s a nice way to start the day, reviewing open things.

Finally, we don’t like letting code review requests dangle for too long. If a PR isn’t merged after a week, we revisit it and see if we even want to pursue merging it. If we don’t plan to merge it for a while, we close it for the time being.

These are some ways we’re able to have a fun, productive & most importantly positive experience with code review and pull requests. I hope this was helpful.

I think the biggest takeaway from here ought to be the good old golden rule: treat others the way you want to be treated.

I’d love to hear your feedback! Thoughts? Questions? Comments? Praise? Love?