Hacker News Reader

Comment by Minor49er | Hacker News Reader

At my former job, the dev team wasn't allowed to use a formatter, so people would often commit code that would be off by a space or three. Similar to the post, I got into the habit of detecting these with this:

  ^( {4})\* {1,3}\S

There were cases where the backend would generate a stack trace, but HTML-format it in a response that was not expecting HTML. I could select and delete them with this, which would make the results instantly more readable:

  <[^>]+>

View on HN

Wasn't allowed to??? A giant hand smote your head if you typed the code format command?

Metaphorically, yes. I mostly built off of existing code while I was there. Frequently (and not just for myself), I would see code getting held up in review because the more tenured developers who looked at it would say that the formatting was beyond the scope of the changes being made and would refuse to approve anything until all of the whitespace was reverted. This same class of devs would also commit new changes with more cases of these annoyances, such as trailing whitespace and mixed tabs/spaces, but were able to merge their unreviewed stuff in without management batting an eye.

This just gave me an idea. I’ve been on teams like this. I’ve also been on teams at the other extreme, where formatting nit picking took up the vast majority of review.

The former is obviously painful because poorly formatted code is a constant mental burden. But it’s also true that unrelated formatting changes can present a similar mental burden and make meaningful changes harder to identify.

The latter can detract from more meaningful review. But prioritizing better formatting consistently keeps a codebase more manageable over time.

This is somewhere tooling can help, more than it already does. Automated linting/formatting is great. Depending when it’s applied, and how effective its automation, it can potentially defer the burden til review time. But there’s still a lot of room for improvement there.

Many diff tools present an “ignore whitespace” option. I would like to see this taken two steps further:

1. Better language syntax awareness could go beyond whitespace as formatting-specific “noise”. I know there’s work in this direction and it’s great too. Besides making it more available and complete...

2. “Ignore” is probably not the best option. Ideally tooling can also identify these two sets of changes so they can be both viewed, and more importantly merged separately.

The problem right now is that formatting is noise pretty much everywhere one might interact with a project. Making it a distinct set of changes at the diff and history level would help significantly.

That would certainly help to curb the problem. On the flip side of that, a couple of things that have helped to stop it before it gets to that level are to ask the developers to enable "auto-trim whitespace" and to enable the option to show whitespace and/or hidden characters. Most editors can do these things. They go a long way toward stomping out the problem, even if auto-formatters aren't being used.

Highlighting trailing spaces are an option in git diffs too, though the devs who submit them typically don't look at their own diffs after they've pushed them (at least in my experience)

I generally dislike getting commits to review with a bunch of unrelated formatting changes around it, so I kind of get that pushback. As a lead I've asked people to format PRs so at least they're separate commits to review. That said, even I set my editors to clip EOL whitespace and add a linefeed on save if need be for every file I touch, so I guess I can't be too rigid about it.

I do think formatters work great (much better than format linters) but only if everyone does them on landing from a shared conf. Otherwise you end up playing format tennis and a lot of expression-normalization noise if it's working from AST.

I think the point where it's rewriting braces, adding parens, etc, is the point where I'd typically want to see the reformat split out for review. I haven't seen a code review tool in a long time that didn't let you hide whitespace-only changes, though, so not sure I think pushback would be justified there if the files were being touched anyway.

It's not uncommon for configurations to be included in a codebase. Anyone new to a project who uses an alternate editor can commit a config with the same settings, too.

One thing we're running into at my current company is that there are pre-commit auto-formatters that are employed that will take care of this for us. It's really convenient. The only drawback is that this process was introduced many years after the codebase was created, so there are still occasional lingering files that will pull in formatting changes once someone changes even a single character. The upside is that this isn't very frequent at this point, and we're kind of expecting them to happen on occasion.

A couple of devs have also been planning on running the entire codebase through the formatter all in one go, which will happen during a lull. I've seen another company do this once, and while the result is nice, it always ends up with someone getting frustrated with having to iron out conflicts in some branch they've been working on. (And of course, another drawback is that it complicates the blame process, but that might not be a huge problem depending on the codebase.)

Reply on news.ycombinator.com