Hacker News Reader

Comment by delusional | Hacker News Reader

I always make it very clear in my comments if something is a blocker (this can't be merged before this is fixed) or a note (this would better fit the style of the existing code), and then i make it very clear in person that I do not take it as a personal affront if they don't fix my note comments. I find that this works pretty well. It has the additional positive effect of opening me up to criticism. If they understand that not all my comments are always vital or "right" I imagine it's easier to dispute anything else I say too.

View on HN

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!

> 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.

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.

Reply on news.ycombinator.com