Hacker News Reader

Comment by arp242 | Hacker News Reader

It's impossible for any code formatter to be 100%. Where to put a blank line? Where to break a line? How to name a variable/function? etc. etc. Some try (e.g. prettier), and what you end up is frankly just bizarre code that's just ugly. Never mind tons of other small things that often don't really matter.

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".

View on HN

> The solution is to just not be too anal about it. It really is a cultural problem

"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

You can't "enforce" all of these things. That was my main point.

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 start telling people off at work, there is a good chance that you will be perceived as being 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

> If you start telling people off at work, there is a good chance that you will be perceived as being the problem

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 agree that good engineers focus more on the actual structure and problems instead of nitpicky things like formatting

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.

style fixes should always be separate commits. if they fix a PR, they should be part of that PR or an independent PR, but ideally not included in the next code changing PR

> style fixes should always be separate commits.

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.

ok, yes, you should not introduce style problems to begin with. but if the problem is already there from an older PR, it should be fixed separately.

This:

> "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.

They aren't the same at all

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%

The point where this is the same is "everyone just needs to agree on a common style".

> 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.

You're not wrong.

But in theory, agreement and buy-in is a one-time thing, while actually writing the code and reviewing the PRs are constant things.

Yes, but aligning the team culture is in theory also a one-time thing. And this is IMHO necessary in any case, no matter if you decide to enforce this alignment using auto-formatters or not.

> It's impossible for any code formatter to be 100%. Where to put a blank line? Where to break a line?

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.

Reply on news.ycombinator.com