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.
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.
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.
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.
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.
And just for reminder, the following is one important point at the beginning of the pep8:
Foolish Consistency is the Hobgoblin of Little MindsOr if that is too much (no shame!), just accept style differences.
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.
It’s also often handy to configure linters and autoformatters as precommit hooks.
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.
And should I stress again that there is absolutely zero positive tradeoffs?
Just because you don't value or acknowledge them doesn't mean they don't exist.
As I specifically said, the CI should fail on the feature branch. If you’re only running CI on master, you’re going to run into the exact same problem if your unit tests fail. The way to avoid that problem is to run the unit tests on your feature branch, and if you’re doing that you might as well run the linters too.
Strong disagree. If it’s not in the CI, it’s optional. Respecting the formatting standards of the project is NOT optional.
It should definitely be applied as a pre-commit or pre-push hook too to make sure the CI step is just a formality, but it’s not enough.
The whole point of CI is to automatically verify the code. Linters are a method of doing that, the same as tests.
> 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.
Sounds like an easy fix to me. And if your CI is set up properly, the misformatted branch wouldn’t have been merged to master in the first place.
Against errors and regressions. Meaning, stuff that breaks your code and affects the service you provide to users.
Style issues ain't that. Come on.