While yes, nit-picky review could mean the reviewer doesn't understand the big picture, an LGTM with no comments at all is more likely to be indicative of missing something.
What you really mean is that nit-picky reviews that don't pick up on actual logic or functionality issues are a smell. Which is true, but isn't usually the case at Google[0]. The chapter on teamwork covers some relevant concepts (like psychological safety).
[0]: https://testing.googleblog.com/2018/05/code-health-understan...
You seem to be assuming, without prompt, that nit-picking punctuation comes at the expense of a thorough overall review.
Code review on punctuation is absolutely an important part of readability. Consistency makes understanding and scanning (for human readability) and potentially parsing and modifying (for machine readability) much easier.
As someone who both reviews a lot of code and reads a lot of code, consistent documentation with good grammar and punctuation is enormously helpful.
People who focus on punctuation do so at the expense of everything else, sure. But people who give thorough reviews give thorough reviews, including nitpicky comments.
I've given and received thousands of reviews at Google (in multiple languages, across multiple teams), and can say that my experience has been fairly consistent: code review is high quality and detail oriented, including, but not at all limited to, small things like grammar and comment formatting.
> But people who give thorough reviews give thorough reviews
Ok.
> including nitpicky comments.
Disagree. Pointing out that someone forgot to capitalize an initialism in a comment is not thorough, it’s a time suck. A thorough and competent reviewer will realize it doesn’t matter and will know not to distract the CL owner will such irrelevant trash.
Sure, there is an a small section of the Venn diagram that leaves good review comments and sprinkles in “nit:” turds, but that group is much smaller than both the fully incompetent nit-pick group and the competent reviewer group.
Seriously, unless a code comment is illegible or it’s being compiled into user-facing docs, it’s absolutely useless to leave something like “nit: it’s TCP, not tcp”.
I think you're describing style errors, not grammar errors. For example, the Google style guide says that (certain) comments should be complete sentences. Asking people to use good grammar (and capitalize/punctuate) those sentences as though they were documentation (because they are!) is valuable for reviewers.
I say this as someone who, when reading unfamiliar code, is able to understand it more readily if the inline documentation is easy to read. "tcp" vs "TCP" doesn't impact that, but other grammatical nits absolutely due.
The entire thread (not started by me) is about nit-picking punctuation and now you’re trying to explain to me that I’m describing style errors. So what? It’s useless feedback regardless of which chapter in the Holy Google Docs covers it.
> Asking people to use good grammar (and capitalize/punctuate) those sentences as though they were documentation (because they are!) is valuable for reviewers.
Right, it’s valuable for the reviewers because it makes it seem like they provided feedback when they have nothing of substance to add. It’s not valuable for the author though nor is it realistically valuable to future people reading the code in question.
>I say this as someone who, when reading unfamiliar code, is able to understand it more readily if the inline documentation is easy to read.
I think maybe you don’t know what “nitpick” means. https://www.dictionary.com/browse/nitpick
If it has a meaningful impact on the ability to read it, it’s not a nitpick.
If the comments aren't important enough to spend care and thought on, they should be deleted.
> If it has a meaningful impact on the ability to read it, it’s not a nitpick.
This is boring semantics. Given what you describe as nitpicks, we draw the line of meaningful impact on readability in different places. So I'm claiming that some of the things you describe as nitpicks in fact aren't.
The closest I've seen to this is comments of the form "can you reformat this" or "this doesn't match the rest of the file". Google's style guides have fairly strong guidance on formatting, so there's usually a "right" way (enforced by a linter), and if not, then usually you defer to the existing style in the module.
I would agree with linting or just deferring to existing style, but in truth I care little about this, it's just not significant to me (yet others seem to overvalue it, to the point of talking exclusively about this).
It's actually nice. I disagree with some of the rules and would like them to change. I think that if your if-block has a single statement and there's no else, and it all fits in a line then it's fine to inline it.
if (theAnswerIsKnown) return theAnswer;
Edit: To be clear, I might be right, and this might be better, but I'm "wrong" in the sense that it's not linter compliant.
Do you mean 'wrong' in the sense of 'wrong according to the rules enforced by the linter' or 'wrong philosophically' ? If it's the latter, how is that wrong and is not even a subject for debate?
This is an honest question, I guess I just don't see how something like this can "just" be wrong (to me it's irrelevant and not worth the ink used to write such statements in code reviews... though I'm playing devil's advocate here obviously :-) ).
PS: of course if pre-submit checks enforce the rules, then as you said it becomes a non-issue. I have been in environments where there are no such tool-enforced checks, yet people comment on it endlessly, routinely blocking code reviews for whitespace issues...
Seriously, code review on punctuation in a comment is not good in any scenario and it speaks nothing to the legibility of the code itself. If anything, it might mean the code is unreadable and the reviewer is too embarrassed to point out that they can’t follow it.