27 ms·

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 beca

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.

zeroCalories 3y ago
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.
- merb 3y ago
  Sadly typos in variable names are not checkable that easy
  - zeroCalories 3y ago
    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.
    - bluGill 3y ago
      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.
      - dgellow 3y ago
        It’s called cspell https://cspell.org/ https://cspell.org/
        johannes1234321 3y ago
        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 ...
        comex 3y ago
        > 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.)
        merb 3y ago
        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
  - kridsdale1 3y ago
    We (at google) do have this checker.
- tunesmith 3y ago
  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.
  - zeroCalories 3y ago
    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.
- er4hn 3y ago
  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.
  - zeroCalories 3y ago
    Have not tried a ton of tools, but Codecov seems to work fine for my projects on Github.
everforward 3y ago
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.

zeroCalories 3y ago

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.

merb 3y ago
Sadly typos in variable names are not checkable that easy
- zeroCalories 3y ago
  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.
  - bluGill 3y ago
    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.
    - dgellow 3y ago
      It’s called cspell https://cspell.org/ https://cspell.org/
      - johannes1234321 3y ago
        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 ...
        comex 3y ago
        > 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.)
      - merb 3y ago
        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
- kridsdale1 3y ago
  We (at google) do have this checker.
tunesmith 3y ago
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.
- zeroCalories 3y ago
  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.
er4hn 3y ago
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.
- zeroCalories 3y ago
  Have not tried a ton of tools, but Codecov seems to work fine for my projects on Github.

everforward 3y ago

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.