Hacker News Reader

Comment by ryanschneider | Hacker News Reader

Good call, I use “minor:” as a comment prefix for this but just realized that I got into that habit awhile back and the team composition has changed since then so newer members don’t have that context.

I wish there was something like semantic commits “fix/feat/chore” for review feedback, specifically a keyword for these little things that are more future advice than must fixes.

I also try to be clear when I don’t expect to do a second review if they make the minor changes, that the green check mark carries forward.

More. broadly I’ve been trying to favor “rough consensus and running code” in my PR reviews, even if I don’t fully agree with an approach I’ll give the writer the benefit of the doubt since they’ve most likely spent more time thinking about the problem than I have. Or at least that’s my hope!

View on HN

I've been known to outright use 'nitpick:' - and if it's a nitpick that matters to me personally will tend to supply a diff for them to grab. Keeping the effort required to resolve it in line with how much it matters to the codebase and/or other developers working on it tends to make people a lot happier about the suggestion.

We use the following:

Nit: This is a minor thing. Technically you should do it, but it won’t hugely impact things.

Optional: I think this may be a good idea, but it’s not strictly required.

FYI: I don’t expect you to do this in this PR, but you may find this interesting to think about for the future.

None of these block a merge and anything beyond these type of comments will be a in-person discussion.

I think for optional I'd just write "It may be worth ..." and I think I might prefer using 'Maybe:' as a tag because 'Optional:' implies to me "I'm sure this is a good idea but it's not strictly required" which is enough different to seem worth separating the two.

The FYI tag is a fantastic idea and I shall try and remember to use it in future.

> I also try to be clear when I don’t expect to do a second review if they make the minor changes, that the green check mark carries forward.

This is all well and good until your company brings in automation to enforce the setting that all changes dismiss reviews on all repositories.

Combine it with the requirement that all branches must be up to date with master before merging for the double whammy of rebase/review/repeat hell.

Or instead imagine that you work in company where you actually can influence the settings of your repos so you can enable or disable these features whenever the team agrees on it. IMHO decisions like these should generally be up to the team, and I generally do turn off the "changes dismiss approval" setting because I trust my team mates not to abuse this.

Reply on news.ycombinator.com