For me this says more about the company culture than any inherent flaws with the code review process.
No discussions of whether
if (foo) return;
if (foo) {
return;
}I agree. Once I had the displeasure of working with a junior dev who was very prolific in posting comments on style and if a space should be at the left or at the right of a symbol. It took me a few days of dealing with that noise to onboard a linter.
Even so the junior dev felt entitled to manifest how high their standards were by posting a torrent of comments in the PR that onboarded the team's official linter config.
But once the PR was approved and merged, surprise surprise: the junior dev's PR comment metrics dropped from dozens per PR to zero. The style guide didn't even had to be enforced.
The last I've heard about the junior dev was throwing a tantrum when one of their PRs received a comment from another team member asking to run the linter because it failed to adhere to the team's official style guide. Apparently the high standards and this attention to detail only went one way.
Add it to your CI also, make lint and a make fmt.
Formatters also do a poor job if your language is flexible and expressive. Great for go, but if your language is the kind that easily supports internal DSLs, formatters are not even helpful at making the code regular
What's your definition of "trivial style nonsense"? Is it "something I personally don't understand or care"?
Things like tabs vs spaces is important, and so is how many spaces an indentation level should take. This affects how editors reformat code, how other people's editors present the code, and even how many lines a commit has and is blamed for a change.
That's why linters are of critical importance to a team, and so is adopting a shared standard and editor config.
The only people who don't understand the importance of a style guide and how to make it a non-issue by enforcing tools to enforce it are those with little to no experience working with software.
Linting and style guides are not of "critical importance". No business objective will go unachieved because some checkins use tabs and others use spaces.
Whatever problem style issues might cause can be resolved before lunch by running a formatter and committing the result.
This is simply false, as attested by the huge volume of comments in this thread by those with actual professional experience working on real-world software projects.
You're also oblivious to the problem domain, because otherwise you'd understand that the critical problems are not whether a space should be at the left or at the right of a symbol, but all the churn that is required to manually address style problems in PRs.
Try to think about the problem. You post a PR that screws up all formatting. It takes time for a team member to review a PR. Once you start to get reviews,you notice comments pointing out failures in complying with a specific style. Whether you go the passive-aggressive path of waiting for any other team member to review your code or you do the right thing and fix the problems you introduced, that requires another round of PR reviews. The time that you take with each iteration is the time your work is delayed to be merged. Now think about how many hours per month you waste just because you can't manage to format your code properly.
I sort of agree with GP in that the discussions are a waste of time. I also agree with you that you should simply automate it through tools. Styling doesn’t have to be a democracy or about personal preference, all styles work, it’s all about picking one and then forcing everyone to use it. Of course you do it in a much more involving process than what I make it sound like here, but ultimately someone with decision making powers is going to have to lock down the process so no further time is wasted on it.
blame.ignoreRevsFile
We did have some outside contractors who didn't get it at first, but after several of their submissions were rejected (with potential financial penalties) they got onboard and followed the guidelines we had sent them. "These people mean it."
I'm curious why you think this would happen... I would imagine that their comments will now have be to about things that matter, and if they are unhelpful they will stand out more.
It is not obvious that an actual code change is of poor quality.
To show that takes infinitely more experience, work to analyse all the direct and indirect ramifications of both the original and proposed approaches, capacity to push back and decide that your own engineering judgement is equal or better than whoever is purporting to correct you, willingness to suffer everyone else accusing you of arrogance for that on top.
Even with all of that, it's a lot harder to prove the valueless comment is valueless because the more you know, the more you know that practically every single line of code could be done 30 other ways with some valid argument for each one.
It's completely insidious both for the junior and the senior.
The junior has no way to know the junk comment is junk. So they internalize the comment and everyone is worse for it.
The senior has it almost worse. They know enough to know they don't know everything and the comment might be valid, so they put in all kinds of work to try to figure out if they actually missed something, did they have point etc. Maybe in the end the code doesn't end up as bad as the junior just rolling over, but it sucked for everyone and the challenge was not really an intentional productive crucible, it was just a douche that everyone had to waste time and energy taking seriously.
Only someone who actually is arrogant (whatever level, whichever side of the pr) has it easy. Again bad for everyone except them maybe.
It’s not necessarily even bad if there is no meaningful leadership above or alongside the junior. The fast-rising junior may end up hiring developers who are more assertive and still capable.
I don't particularly care which one you choose - although I admit to having a preference - but I want that style consistently applied throughout the project, so I'm not stumbling over stylistic differences as I'm scrolling through code trying to investigate something.
And keep in mind that this particular example, that some people can read clearly, is just five lines. When you're looking at a 3k line file, one of the dozen you're digging through during an outage, that shit matters.
The second form is superior for at least three reasons:
1. Enables a property corollary to the happy path rule, which is that every return statement of a function is at the beginning of a line. This property is crucially important for readability, it makes it reliably possible to determine every exit point of a function by merely scanning the left hand vertical slice of it as opposed to needing our eyes to jump around erratically searching for arbitrarily placed returns.
2. Uniformity: mixing different if statement styles in a single codebase interferes with our ability to match visual patterns. Since we can no longer rely on every if statement looking the same, we cannot quickly skim over code anymore.
3. Refactorability: it's easier to add another line of code to the if statement if there's no need to also add brackets.
I don't remember the details but I've had some typo in a one-line-if-condition once and it took me days to find it. Might have been an accidental semicolon in C. A linter enforced the braces and line-breaks and made it obvious.
Anyways, running a linter also helps because all anger or frustration can be directed towards the machine.
Did you mean “not having a linter”? Because once you have a linter you no longer need the braces because the autoformatting will always fix the indentation.
if (foo)
return;
if (foo)
doSomething();
return;
A linter would enforce having braces and solve the issue. And for codebases without a linter it is a good habit to just go with the braces.
No style could save us from those devils!
People should take the necessary attention to make quality work (and use proper techniques, where proper does not entail style or other visuals or appearances, not at all. That's for superficious persons). This is true for any part of life not just coding.
One man's "stylistic nitpicking" is another man's violation of the company's official coding guidelines.
Does the company adopted a linter? If so,why has the new hire not applied it before posting the PR?
Imagine a new hire throwing a hissy fit because even though the whole company writes code in PascalCase he feels that snake_case is better.
Imagine a new hire decided that tabs are better than spaces, and proceeded to reformat 30% of a source file leaving behind wonky indentation.
Shall we tolerate code that has both just so that random internet people in a random online forum can rail against PRs?
Or shall we post a comment in a PR pointing why the code should be reformatted?
It's neither new nor novel. Even GitHub allows anyone to post a commit on a feature branch, regardless of who created it.
It's not a GitLab or GitHub feature. It's a Git feature: the ability to post a commit to a branch.
https://github.blog/news-insights/product-news/suggested-cha...
git reset --hard HEAD~1
git push -f
:)
https://docs.gitlab.com/ee/user/project/merge_requests/revie...
https://docs.gitlab.com/ee/user/project/merge_requests/appro... and https://docs.gitlab.com/ee/user/project/merge_requests/appro...
And now you contributed to turn your team culture into shit, with a toxic mix of PR creators purposely shitting on the team's style guide and PR reviewers who have to constantly post follow-up commits to your work because you can't even put together and acceptable PR or clean up after yourself.
What if anyone who posts a PR addresses feedback anyone posts on their work and addresses them until you get the necessary and sufficient approvals?
How does that inform the PR author that they failed to pay attention to a relevant detail, and more importantly contribute to not make it again?
If there is no feedback, problems will persist.
Nit comments add noise and failures to comply with a style guide is always a distraction on a PR, but that does not mean style guide violations should be ignored. It means that your team must work their way into making them a non-issue.
This means establishing an objective and usable set of rules and guidelines, and enforce them programmatically with a linter, not turning a blind eye to style violations.
And any manager that lets it happen without severe reprimands to the time-waster isn't worth working for.
We've all got way more important things to be spending our time on.
That would be something only a terribly incompetent manager would ever do.
A manager who is not terribly incompetent would either not take sides until explicitly asked to weigh in, and if and only if he was dragged into the discussion then the manager's only acceptable input would be to eliminate the problem for good by adopting a linter.
Everything else is a mistake and a complete waste of time.
Passive-aggressive minefield. Someone will see a change done to THEIR code, without the changer even asking, and it will feel like the person who did it is a passive aggressive dickhead. Resentment will brew, tempers will be lost. It will only get worse from there. Software engineers already aren't exactly known for their humbleness and ability to swallow their ego.
That culture will turn even the most conscientious dev into a monster.
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.
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.
And should I stress again that there is absolutely zero positive tradeoffs?
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.
Just because you don't value or acknowledge them doesn't mean they don't exist.
I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter).
On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you to do your best work. You will be essentially blocked pending their approval (so that nitpicks are changed according to their likings).
The weird thing is that all this traceability leaves traces for management to see who's doing a good pr review job and who's not... But I've learned that management usually does not care much.
In my current role I review a lot of PRs, they tend to be large due to the waterfall way things work... If I don't ask to fix the spelling now, It might not happen for a year or two. That said if it can be fixed in separate PR before release, that's fine.
Really, it's best to have some terms or explicitness.
For example, with my teams 'Nitpick' means I'm just being nitpicky, Doesn't have to change unless it's on the edge (and I'm explicit as to why it should change, i.e. I know the next thing will need the change anyway). "Consider" means It doesn't have to happen, but here's some food for thought. "PLZ FIX" is fairly self explanatory.
Also, making sure management (especially for contract houses) knows that PRs are a 'judgement free zone' and should not be held against people for perf reviews etc; that should be collected by other peer feedback channels instead.
I always thought this was the best way.
I wish these systems had a way to assign severity to comments, and urgency to the commit.
If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.
Good pr systems will perform a build of the code and run tests, so if the mis-spelling is in the code the build will fail anyway.
I would even accepts smaller issues (style) when there is a bigger issue to fix. People might eventually outgrow it.
The code is the same way and there was a great comment higher up about an ignored lint rule being an indicator of someone thinking about it and making a judgement call that this time the rule shouldn't apply. I have SOME peers where that my reading of that line of code and others where my assumption is the opposite and they were just lazy or didn't know how to do it "the right way". It's the same line of code!
I don't know how tooling will ever replace this part so I think it's important to keep it as the bedrock of any collaboration process.
Posting PR comments doesn't prevent you from accepting the PR. You can post a torrent of nit comments and still approve it so that minor issues don't turn into blockers.
Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
If your PR doesn't pass lint checks, it doesn't get merged. And the only reason it would fail the lint checks is if your pre-commit hooks didn't fire.
There is no argument of 2,4,8 space vs tabs, because the code you commit is run through the linter.
Write however you want for the things that don't matter, the formatter always wins.
The solution is to just not be too anal about it. It really is a cultural problem.
For example a few weeks ago I reviewed a PR from a new team member; there were some seriously structural problems with his approach, so I commented on that and ignored all the small stuff for now. Another programmer on my team also reviewed at the same time, and only commented on the small stuff that, IMHO, don't really matter, and didn't look at the general approach at all (which really was just all wrong, and also quite obviously wrong).
Not to be too arrogant about it, but I feel this sort of stuff is what distinguishes a "good engineer" from a "mid engineer".
"Any proposal that requires everyone to just is not a solution, because everyone will not just"
People are anal. You aren't going to get them to stop being anal
A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter. If anyone cannot come to an agreement with the rest of the team, or continues to be anal about things that do not appear in the style guide after this, then that person has singled themselves out and the company will need to find a way to deal with the behavior
I agree that good engineers focus more on the actual structure and problems instead of nitpicky things like formatting
That said, code cleanliness and consistency is important too. It makes codebases much easier to maintain and understand if everything is formatter consistently. It's a pretty mid engineer take to think it's not important at all
And you absolutely can stop people from doing that. Simply accepting dickish asshole toxic behaviour as "well, people are like that shrug" and never telling people off is exactly the problem.
If you go to management to complain about a coworkers behavior you may just be told that you have to adjust your expectations to get along with them
If there's a team agreement and someone continues to violate it then you actually have a complaint you can make to management that has some bite to it
The whole concept of a PR is to review the changes you ask your team to pull into the repository.
What do you think PR comments are intended to be? Pats on the back and public announcements on how awesome you are?
No, the whole point of a PR is to allow others to review the changes you proposed so that the mistakes you are trying to introduce are easier to spot and prevent.
What do you call comments that flag a problem with your code changes? Do you call it "being the problem"?
> If there's a team agreement and someone continues to violate it (...)
How will they tell if you do not point out those violations in the PRs? That's precisely why they exist.
I don't think you understand that formatting is of critical importance.
Sure, your code won't break if you add a space at the right or at the left of a symbol.
But your code will be reformatted the next time someone like you works on that file and takes the same naive approach to that code that you took.
That leads to PRs having a larger code footprint for no reason other than fixing the previous PR's failure to comply with a style guide.
This means tools like Git blame start to flag parts of the code as having changed recently just because you failed to pay attention to the style guide.
Now that regression that was introduced by an unrelated commit becomes slightly harder to track because it's buried between commits that add and remove white spaces around the problem, and the last change is just nitpicking around something you should have gotten right in the very moment you posted your PR if only you ran a linter or paid attention to the comments posted in your PR.
Not really. If you're already changing the code and running linters afterwards introduces changes over your change, this means you are the one introducing the problems. Separate commits just add noise.
Your comment is like saying that bug fixes should be separate commits when arguing about how not to add bugs to begin with.
> "Any proposal that requires everyone to just is not a solution, because everyone will not just"
invalidates this:
> A real solution is to have the team agree on a shared style guide, then enforce it with a linter and formatter.
... since the "team" is everyone. It's basically the same problem.
The other issue is that linters/formatters don't "solve" all formatting/stylistic choices. Most formatters, fortunately, still allow you to choose where you do line breaks for example, since they do matter and shouldn't be arbitrary.
A shared style guide is an external impetus to adjust behavior. It comes with external accountability, and also an implicit understanding that violating the shared expectation may bring consequences
"Everyone should just be less anal" is expecting an internal impetus to adjust behavior. There no external accountability, and there's no expectation that failing to do so has consequences
> The other issue is that linters/formatters don't "solve" all formatting/stylistic choices.
80% of a solution is better than 0%
> 80% of a solution is better than 0%
I'm not sure if it's 80% or rather 20%, I guess it's a POV / arbitrary number.
In any case, this is far from a solved problem, while I often see in these type of discussions the idea that auto-formatters solve formatting/code style problem. "Just use black" while dismissing the idea that the rest has to be tackled informally / culturally.
But in theory, agreement and buy-in is a one-time thing, while actually writing the code and reviewing the PRs are constant things.
Not true. Those are objectively covered by any linter.
> How to name a variable/function?
Unless the issue is things like snake_case vs PascalCase, that's not the job of a linter. That's exactly what PR comments are about.
> Some try (e.g. prettier), and what you end up is frankly just bizarre code that's just ugly.
Not true. Without Prettier you always get bizarre code that's just ugly. You don't notice because you paid no attention to the code you wrote, it followed your personal subjective opinion you happened to have at that moment, and you didn't felt strongly enough to verify it.
Without a linter, others would certainly point out the problems they saw in your PR as that would certainly not comply with their personal subjective opinion they might hold at that moment and not before or after.
The importance of Prettier is that it objectively enforces a set of rules. If it's ugly it's because you configured it to be ugly, but it's less of a problem because it will be objectively, systematically and reproducibly ugly.
Adopting a linter does not eliminate style issues. It just eliminates the number of possible comments a PR can get over style issues by replacing all detailed feedback with just one comment with a very clear actionable request that's trivial to satisfy: "It seems X doesn't look right. Could you please run the linter?"
But only use a linter if you will add your rules over the empty set. If you get a pre-built set and are expected to remove the ones you don't need, you are making a toxic environment.
If you continue to have people bringing up arguments over the linter and formatter after an initial agreement is made, then you can talk to those people