3 ms·

This kind of feedback has largely been solved, for me / my team at least, with linters and formatters (as mentioned in the article). So I'd say it is still a re

This kind of feedback has largely been solved, for me / my team at least, with linters and formatters (as mentioned in the article). So I'd say it is still a reflection on the code review process being broken.

SketchySeaBeast 2y ago
Yeah, if you care about it enough to stop a PR because of something and that something can be automated, automate it.
johannes1234321 2y ago
There is a lot of stuff a linter can't do, which however are stylistic choices some people care a lot about. Naming being a big one. Which terms to abbreviate how etc. Also there are a lot of things where automatic highlighting is possible, but any rule needs exceptions which a re subjective (nesting, usage of "raw" looos, early exit, ...) Trying to get some consistency there can be good. And teaching new team members the "habits and traditions" may simplify long term maintenance. However could review tools aren't a good place for distinguishing between "hey, this is good, but I'd like this slightly different" from "there is an actual issue" but that has to be solved somehow by communication. Which often isn't the best skill for engineers.
- chipdart 2y ago
  > There is a lot of stuff a linter can't do (...) Whatever a linter can't do, it's not worth doing. > Naming being a big one. You're confusing personal opinions over how to name things with concrete style issues.
roland35 2y ago
Yes exactly! Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. Or if that is too much (no shame!), just accept style differences.
- chipdart 2y ago
  > Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. What if you post a PR and forgot to run a linter or configure your editor? Should that not justify a comment over style violations? The problem will only go away if everyone is on the same page.
  - philwelch 2y ago
    CI should reject the feature branch if the linter fails. Never waste an engineer’s time doing work a program can do. It’s also often handy to configure linters and autoformatters as precommit hooks.
    - chipdart 2y ago
      > CI should reject the feature branch if the linter fails. Your CI pipeline is broken if it refuses to run because of style issues. Linting is either applied as a pre commit hook or manually by the developer. Anything else is a mistake you are making without any concrete tradeoff. The same goes for other mistakes such as handling warnings as errors. Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4.
      - nemetroid 2y ago
        Just make sure that the pipeline is fast, or allow an override, and there is no issue.
        chipdart 2y ago
        Requiring manual intervention to handle a blocked pipeline over a non-issue defeats the whole purpose of continuous integration, not to mention that you are suggesting adding twice the complexity as an alternative to not adding any complexity at all. And should I stress again that there is absolutely zero positive tradeoffs?
greatgib 2y ago
In lots of case linters might be harmful also. It is one thing to have a style guide people follow in most cases, it is another one to rigidly force everywhere the style that the linter enforce. Especially because most linters will impose that everyone on the team will use them. For example, if we take Python, there is black that is bat shit and makes your code less readable but adopted by so many teams as cargo cult because doing that signal that you are a cool, hype, best practice following team... In the end with "black", people are forced to abide by the style of the random guy that created it, that is not exactly following the PEP8 or the zen of python, and is just itself created in cargo cult inspired by go and rust, and Python creator even advised against using it for readability except in special cases like very very big teams.
- jessekv 2y ago
  > not exactly following the [...] zen of python Blacks's stance that "there should be one obvious way to format things" seems consistent with the zen, even if you disagree with the actual rules.
  - greatgib 2y ago
    For me it is not respecting the zen of python by pushing you to bad formatting sometimes just because the tool requires you to. And just for reminder, the following is one important point at the beginning of the pep8: Foolish Consistency is the Hobgoblin of Little Minds

SketchySeaBeast 2y ago

Yeah, if you care about it enough to stop a PR because of something and that something can be automated, automate it.

johannes1234321 2y ago

There is a lot of stuff a linter can't do, which however are stylistic choices some people care a lot about. Naming being a big one. Which terms to abbreviate how etc. Also there are a lot of things where automatic highlighting is possible, but any rule needs exceptions which a re subjective (nesting, usage of "raw" looos, early exit, ...) Trying to get some consistency there can be good. And teaching new team members the "habits and traditions" may simplify long term maintenance. However could review tools aren't a good place for distinguishing between "hey, this is good, but I'd like this slightly different" from "there is an actual issue" but that has to be solved somehow by communication. Which often isn't the best skill for engineers.

chipdart 2y ago
> There is a lot of stuff a linter can't do (...) Whatever a linter can't do, it's not worth doing. > Naming being a big one. You're confusing personal opinions over how to name things with concrete style issues.

roland35 2y ago

Yes exactly! Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. Or if that is too much (no shame!), just accept style differences.

chipdart 2y ago
> Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. What if you post a PR and forgot to run a linter or configure your editor? Should that not justify a comment over style violations? The problem will only go away if everyone is on the same page.
- philwelch 2y ago
  CI should reject the feature branch if the linter fails. Never waste an engineer’s time doing work a program can do. It’s also often handy to configure linters and autoformatters as precommit hooks.
  - chipdart 2y ago
    > CI should reject the feature branch if the linter fails. Your CI pipeline is broken if it refuses to run because of style issues. Linting is either applied as a pre commit hook or manually by the developer. Anything else is a mistake you are making without any concrete tradeoff. The same goes for other mistakes such as handling warnings as errors. Imagine going into a meeting with a senior manager and explain that you cannot release a hot fix because your pipeline is broken due to the last commit having 5 spaces instead of 4.
    - nemetroid 2y ago
      Just make sure that the pipeline is fast, or allow an override, and there is no issue.
      - chipdart 2y ago
        Requiring manual intervention to handle a blocked pipeline over a non-issue defeats the whole purpose of continuous integration, not to mention that you are suggesting adding twice the complexity as an alternative to not adding any complexity at all. And should I stress again that there is absolutely zero positive tradeoffs?

greatgib 2y ago

In lots of case linters might be harmful also. It is one thing to have a style guide people follow in most cases, it is another one to rigidly force everywhere the style that the linter enforce. Especially because most linters will impose that everyone on the team will use them. For example, if we take Python, there is black that is bat shit and makes your code less readable but adopted by so many teams as cargo cult because doing that signal that you are a cool, hype, best practice following team... In the end with "black", people are forced to abide by the style of the random guy that created it, that is not exactly following the PEP8 or the zen of python, and is just itself created in cargo cult inspired by go and rust, and Python creator even advised against using it for readability except in special cases like very very big teams.

jessekv 2y ago
> not exactly following the [...] zen of python Blacks's stance that "there should be one obvious way to format things" seems consistent with the zen, even if you disagree with the actual rules.
- greatgib 2y ago
  For me it is not respecting the zen of python by pushing you to bad formatting sometimes just because the tool requires you to. And just for reminder, the following is one important point at the beginning of the pep8: Foolish Consistency is the Hobgoblin of Little Minds