Hacker News Reader

I often say that “nit” comments on prs are bad. It’s either important enough to make a linter rule, or not important enough to mention.

View on HN

I wouldn't go that far - to me what's important is that nits must be non-blocking. I.e. the author is free to ignore them if they disagree, and a PR review with only nit comments should be an approval. If those things are true, it's fine to have gray area between what's linted and what's mentioned in PR review.

+1

For example, "nit: maybe call the function updateCreditCard instead of updateCard"

If you disagree and think your function name is better (or that the two are equally bad), then I'm happy to go along with what you've got. But maybe you didn't think of this name or maybe I've convinced you. Either it's a quick fix (and no re-review needed) or you just dismiss my comment.

100% agree! The pattern that I've found works well is:

Reviewing a PR with "Approval" status means that the comments are just suggestions.

Choosing "Comment" status means that the comments are optional but important enough that I want to make sure that you read them. If you come back and say "I read them but decided not to make any changes" then I'm happy to approve the PR.

Choosing "Request changes" means that the comments aren't just nits.

Reply on news.ycombinator.com