Hacker News Reader

Comment by MarkLowenstein | Hacker News Reader

Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion. You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style guidelines, either put it in the guidelines, or hold your tongue.

View on HN

The only exception that I would tag on to that is documentation. "I can't understand this documentation/comment" deserves more attention than I think most places give it.

It doesn't directly relate to execution today, but it might later on when someone misunderstands the docs or just gives up on them and tries to hack it together blind.

IMO you should never provide feedback that can be implemented as an automated check. If you don't like deeply nested control flow, then you should catch that with static analysis. If you need code coverage, you should require it for merging. Implement your check and provide a new PR to fix your nitpicks, or shut up. The goal is to put 100% of the focus on correctness.

Sadly typos in variable names are not checkable that easy

Doesn't seem hard to me? You could easily run a spell checker on identifiers and comments. It will produce a lot of false-positives, but that can be solved by making the changes optional or using an allow list.

Then please write one.

Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that might be just that I don't have good ideas: if you create a good tool for this I'm willing to be proven wrong.

It’s called cspell https://cspell.org/

Does that understand that when talking about HTTP headers I talk about "referer" but when talking about JavaScript I have to use "referrer"? - What is wrong in one place can be different elsewhere. Terminology, spelling, accepted abbreviations depend very much on context. And sometimes even a wrong spelling is right as it's in some standard ...

> Does that understand that when talking about HTTP headers I talk about "referer" but when talking about JavaScript I have to use "referrer"?

I bet GPT-4 understands that.

(Though I have no personal experience using it for code review.)

cspell works as a guide, but not as a rejection/auto correction. It’s okish but it does not solve intent. It‘s perfect for comments tough

We (at google) do have this checker.

This seems precisely backward to me. Programmers are humans, not input/output machines, and there's definitely a role for encouraging a certain standard of judgment that doesn't require tooling to enforce. To argue otherwise seems akin to arguing that any bad behavior is fine that isn't explicitly banned in paragraph 5 subsection D. Tooling is expensive, especially for smaller teams, and should be saved for phenomena that hit the cost/benefit calculations squarely.

I agree that sometimes the cost/benefit analysis doesn't make sense for adding tools, but I would argue that in many of those cases the benefit of your nitpicking isn't worth it either, so just don't say anything so that we can focus on the important parts of the code. Of course people will sometimes do some really silly stuff that couldn't realistically be checked, so I would hope that less important stuff gets put aside so we can focus on the real issues.

On that note, what good code coverage tools are out there? GitHub and Gerrit, as well as (egads) ReviewBoard don't seem to have native support for this in the review. It's unfortunate since it seems super useful to have be up front and visible.

Have not tried a ton of tools, but Codecov seems to work fine for my projects on Github.

Reply on news.ycombinator.com