Ask HN: What tone to use in code review suggestions?

I tend to be overly “nice” in code reviews and phrase things more like suggestions and provide some justification for them:

> have you considered x? It is standard practice in y to do z.

> should we do x instead? I think it would be better in the long run because y.

If there is a more serious issue, I prefer DMing the author and hashing things out synchronously.

I think it is important to phrase things gently as much as possible, as tone is hard to read from text, and being overly harsh might discourage other teammates from taking risks and asking for feedback.

Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes

Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…

> Why did you do it this way instead of X?

Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor.

You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already.

What wouldn't wind me up?

"I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?"

Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob.

Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear.

Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.

I absolutely get that. I was a bit in a hurry when I wrote the above comment, and therefore didn't get around to better detail how I use those sentences.

It's all about understanding the context and not talking down to the author.

When I see obvious improvements I use the "order" style and just state what needs to change. Because surely the author can see that the change will be for the better.

The other sentences are for bigger more complex changes where I'm curious about a certain decision. Most often I don't agree on the current design, but I would never write such a comment unless I'm ready to be convinced otherwise.

When someone has spend hours on some functionality and I spend just 5-10 minutes skimming through the code, there is a very real chance that the change I'm going to propose was already considered. It would be arrogant and condescending to just conclude that my idea is better, so I open the door for the authors explanation.

thought: reading this style of sentence would get annoying quickly.

I don't get the point of these TBH. The usefulness of conventional commits is that they can be easily aggregated to get an idea of the types of changes that were made.

Code review comments are purely meant for human communication, so requiring a specific structure is both annoying to write and to read.

I tend to use one, at most two prefixes. "Nit", and rarely "blocker". Though I'd rather specify if something is a blocker once I make my case about what needs to be changed.

Every other comment is assumed to be a suggestion, and depending or not I block the PR by "requesting changes", it's up to the author to either accept my suggestion or provide reasons not to.

Any other strict guideline about how to write prose is silly to me. Especially the decorations section. C'mon now.