Things like that I'd much rather leave as comments in the code, rather than dangling as off-hand things in some unrelated PR. Probably no one will see those PR comments unless they go splunking for some reason, but having them right in the code is much more evident to all the people passing through, or even reminding yourself in the future too.

In general I feel like PR reviews are basically reviews happening too late, and if there is a lot of stuff to review and agree upon in the PR, you'll be better off trying to reduce that up front by discussing and noting design decisions before the work even starts. If there is so many unknowns first, do one step to figure out how it can be done, then agree on that, again before starting the actual work.

That leads to PRs basically just being spot-checking, not deep analysis nor even space for nitpicks. If you as a reviewer spots things after the implementation rather in the discussion beforehand, that ends up being on both of you, instead of just the implementer who tried to move along to finish the thing.

One thing not mentioned is how important it is to acknowledge the comments too. People are taking their time to review your PR, and not even giving a reaction will make the commenter question whether or not it was even received. I'm not looking to throw my thoughts out into the aether. That's what microblogging platforms are for!

I can't tell you how many times I got no response/acknowledgement on a comment that actually surfaced something critical. I haven't been keeping track, but I think my comments could have prevented dozens of outages at this point. It's quite exhausting. In my own experience, the worst offenders of this are senior devs.

> Why approve, if I’ve left comments that I think are worth implementing? > > Because I trust my team. I know that my comments will be considered, and if they’re useful, implemented.

I do this a lot too. It's critical that PR authors don't burn that trust either. If they make substantial changes that warrant another review, I hope they do request it. Too many times in my career have colleagues just went ahead and made bad changes after my approval that I would have easily caught, merge, and things go

High trust, high alignment environments move so fast, and you know when you're in one and know when you have your colleagues' trust. It feels really good!

I think raising the floor, gives diminishing returns once someone is used to the team and the code base, but the conversation always remains relevant. Sometimes teams that resist things like PRs (e.g. "they just slow us down") are actually teams that are having those conversions elsewhere (in-person, on slack, during standup or sprint planning, etc).

If anyone at GitHub is reading this, I’d love a fourth checkbox in the “leave a review” modal that is “Approve but disable auto merge” (alongside Comment/Approve/Request changes)! Even just surfacing “this PR has auto merge enabled” near the Approve button would be great.

To make it easier for myself to leave the CC tags ("nit(non-blocking): ", etc), I use the macOS text expander in System Settings and created mnemonics to easily insert them.

Example: If I type "+pnn", that maps to:

+p = pull request comment n = nit n = non-blocking

Example 2: If I type "+pc", that maps to:

+p = pull request comment c = chore

(and it's blocking because I didn't type "+pcn", the non-blocking version).

Code review has value, but I don't think we are always honest about the costs. At most places I've worked, there has been an informal understanding that code should be reviewed within 24 hours or so, but there is a world of difference between getting a review within 2 hours and 23 hours.

If you have to go back and forth a second time, it's so much more likely that the approval comes much later due to straddling end-of-day in someone's timezone.

Tangentially, if you are designing policies for code review at your org, try to think both about minimum requirements (e.g. PRs should get a first look within 24 hours) and typical requirements (e.g. most PRs should get reviewed within 2-3 hours). What typically happens is what determines overall velocity for your org, so it's much more important than whatever strict SLA policy you have. These are also fixable problems if you have targeted conversations with people. E.g. "I noticed X, Y, Z times, you had unreviewed PRs at the end of the day. Can you set aside some time to review code before EOD? Or try installing PR reminder?"

But PR discussions about lintable style issues always surprise me. The ideal solution is to add a rule in the linter. But when the team won't agree on the rule, and is open to multiple styles, the author should decide, simple! Had a team mate recently who'd block PRs over T[] vs Array<T>, forcing people to stick with Array<T> for simple types like number[] even though TypeScript's own docs and tooling push T[].

There’s no use making the customer wait for your questions, code style suggestions etc to be addressed.

Even if you request changes, you leave all your comments and make explicit which are the blocking ones and which can be addressed in the future.

The 'auto merge on approval flag' PR authors can flip on GitHub breaks this flow though, as it will just merge as soon as you hit approve.

I’m skeptical. Software is as buggy as it ever was. I come across teams shipping terrible quality software, where every line change was approved and reviewed. I come across teams that require every change have an approval, but don’t require 100% test coverage. I’m seeing senior engineers have to get approval from juniors for a copy change.

It’s theatre. It’s bad management. It’s a cargo cult. It isn’t actually driving code quality.

Code review is one thing. Code review is good. But requiring every change have an approval is something else.

Whether those comments get read once approved, I don't know.

Most people are fine, some routinely abuse it to do much larger changes that should normally imply full review - double check afterward! Your name is still on it, it is still your responsibility.

I fully agree with this method. In fact, my surprise mostly stems from the necessity to write this article. So there are places where comments on PRs are avoided as they are considered “unapproved”? Man oh man.

The issue with nitpicks: different reviewers have different definitions of what a "nit" is.

I have now commented on it.