Conventional Comments

off-topic:, suggestion:, consider:

I use nit, but usually for not necessary changes. If my only reviews on a change are nits, I leave the comments but give the greenlight for merging the change (at which point the author may ignore the nits or address them at their discretion)

I'm also not a fan of this phrase, which is whipped out to be "helpful" in reviews at my current employer--probably after the last time this website or one similar to it bubbled up on HN.

To be clear, the (American) dictionary definition of nit-picking is "minute and usually unjustified criticism." In other words, this is definitely not a positive thing to do, which is why this word has such strong negative connotations. Who wants to be nitpicked?

Nit-picking is not a "necessary" change, which is what makes it an exercise in finding the tiniest of fungus flies lurking in a proposed change. "I don't really want to think too hard about what you're proposing, or what your pull request contains, so here's a nit I noticed while examining the bike shed in front of your nuclear plant. #didmyjobboss"

I don't know what a trivial but necessary change is, but that's certainly not a minute criticism.

Someone's it's good just to butter someone up with an obvious but true bit of praise. It makes them feel better about themselves and they also like you more as a person because you said something nice.

For example, if I told my coworker in a meeting that I was impressed by how smooth her deployment went that morning, even though we both know it wasn't hard to get it right because there wasn't much new, it still makes her feel better about her role at the company and how professional she is, clearly motivational, and maybe later she is more inclined to help me out when I've hit a wall on my project, which makes us both more efficient. It also helps her to know I'm not a jerk, so later if I give her criticism she knows I'm trying to help her because I'm not always just the guy who criticizes, I also say positive remarks.

Clearly it has to be genuine praise, but I'm sure everyone can find one thing to praise in any PR.

On a previous engineering team, I once proposed a comment tagging convention that evolved over time to a stable place and ended up being quite useful for us. Unlike Conventional Comments, though, our main motivation was to communicate _expectations_ clearly, so the person receiving the review has enough information to decide what _action_ to take.

The Conventional Comments labels help a little, but requests for action are lumped under "suggestion" and "nitpick" and "chore", which I don't find to be useful distinctions. "Suggestion" doesn't tell me whether the reviewer is just offering an idea for improvement or pointing out something that must be addressed, and doesn't tell me whether I need to check back with the reviewer before merging my change. If I get a "question", then after I answer the question, is it okay to merge or not? "Issue" doesn't tell me if the issue is big enough to block the PR.

——

Here's our system:

• [help] means "I need help understanding this PR". I can't do a useful review of this PR without a better understanding. Please talk to me before proceeding.

• [fix] means "fix now". I've identified a problem; please fix it before merging. If you don't agree that this needs to be fixed, or you think we should do something different from my proposed action, please discuss it with me before merging -- we probably have a significant difference in understanding that is important to sort out.

• [fix?] means "fix now if bug". This looks like it might be a bug, but I'm not sure; please evaluate it, and if it's indeed a bug, treat it as a [fix].

• [minifix] means "fix now if small". I'm asking for this to be fixed before merging because I think it will take • [postfix] means "postponable fix". This is important enough that it must be fixed, but it doesn't need to delay urgent work. You can decide if you want to do it before or after merging this PR.

• [cbb] means "optional, objective" ("could be better"). This could be better in the way I'm suggesting, but it's not so important that it must be fixed before merging. I'm making a recommendation as a good practice for next time.

• [taste] means "optional, subjective". I'm suggesting an improvement, and I'm acknowledging that it's a personal preference. I am not demanding that you change your preferences long-term, but I'm offering it because I find this practice useful and think you might find it useful too. If you like it, use it.

• [fyi] means "for your information". No action needed. This might be mentoring advice, or a reference to something else that's relevant, or a thing to note for the future, etc.

——

I like this system because each tag makes it very clear exactly what action is required before merging and under what conditions there needs to be further discussion with the reviewer.

[help], [fix], and [fix?] are blocking: the PR cannot be merged as-is without discussion. For [fix] the communication loop must be closed with the reviewer; for [fix?] the communication loop can be open.

[minifix], [postfix], [cbb], and [taste] are non-blocking: each one identifies _why_ the suggestion was made so the receiver can decide whether or not to take action. In practice, we found that these 4 categories did pretty well at covering the most common reasons behind suggested actions.

Notice: [fix] and [minifix] aren't about whether the change is big or small. A [fix] could be big or small; the important information is that it's mandatory. The smallness indicated by [minifix] is relevant only because the smallness is the _reason_ for the suggestion.

Notice: There's no [chore] or [style] or [nitpick] category. [style] doesn't help, because we still don't know whether the style change is mandatory or merely recommended. It doesn't matter what kind of thing we're asking for; what matters is what needs to be done about it.

I feel like if this is an issue, there's a larger issue in the team. I've only ever witnessed language be an important issue if people felt overwhelmed, stressed and treated badly. On the other hand, when people feel treated fairly and are happy, most don't care for niceties and prefer straight to the point communication.

Especially rewording change requests as questions is something I don't like. Is this something that should be done? Is it just a question about the technical possibility? A request for comments about the intended change? (I've seen them all and the more friendly they are worded, the harder they are to tell apart)