Hacker News Reader

Comment by neilv | Hacker News Reader

Remember not to mix formatting changes into commits with semantic changes. :)

It's a good rule of thumb, but it also feels like people use that as a tactic to continually change code to get away from having a formatting change come in. That's why on any new project, even personal ones, I enforce formatting from day one if the language has a formatter.

It's pretty annoying to work in a codebase that had no formmatter running so that I then have to make special adjustments to my editor to stop formatting on save for certain files.

I don't understand your point... how would continually changing code avoid formatting changes? If you want to change the formatting, just commit a change to the formatting.

If you're making a big commit that formats a large file or files and those files are constantly getting merged in, then it makes the formatting PR hard to get in as it constantly has to merge or just start over. This might be only a concern for certain scenarios in large companies.

Because your change and the formatting change are probably dependent, and between the time you submit the change and the time your PRs are accepted there may be multiple other commits to that file which affected lines that your formatting PR addressed. If changes wait for weeks to be merged (which is actually pretty quick compared to years) you wind up with the formatting change being blocked on needing a rebase. The thing becomes an annoying PITA of merge conflicts and rebases.

So either have code formatting standards to start with which are automatically applied, or else accept PRs that have whitespace fixes and merge them promptly (I can accept splitting up the commits into formatting and non-formatting commits but even that is barrier to entry over people being able to submit changes to your software easily--every bit of fussiness is a tax on the submitter's time and will result in less submissions).

You make it sound like both of these changes have to be entirely separately reviewed, but most of the code review processes I have seen--including mailing lists--have the ability to merge in a sequence of patches in a single go, rather than having to review each one by one.

Otherwise, if the problem is that you have patches waiting to be merged for weeks that result in merge conflicts, that isn't solved by putting the patches together into a single patch: you just have a process that is going to make formatting changes impossible to clear without tons of extra work.

As for submitters, they need not put in this time: the maintainer or review team can trivially split the patch as this is literally close to hunk selection... it is so easy that I routinely do this as part of every single commit I have made even locally and even temporarily for as long as I can remember?

Regardless, I still don't understand: nothing you are saying makes it sound like the people in your scenario are actively making code changes to avoid having to make a formatting change--"a tactic to continually change code to get away from having a formatting change come in--which is what was claimed and what seemed so utterly confusing... I can't imagine someone saying "I'm going to change this logic few days as I hate formatting patches".

You're assuming a relatively high degree of competency and available time on the part of maintainers and it sounds like a reasonably short backlog. That's great. For overworked, understaffed teams like a large backlog of reviews it generally doesn't work like that. And for those teams the work done on fixing the formatting issues across their project pays off in velocity because submitters and reviewers stop needing to care about the issue entirely.

This is why when people think about submitting a change and they see their editor auto-formatting changed half the lines in the file with whitespace fixes, they get a bad feeling about how submitting anything to the project is going to go.

> how would continually changing code avoid formatting changes?

It makes every single commit as ugly as this one. In other words, instead of only some of your commits being full of formatting noise, all of them are, because touching any line causes rewrapping/reindentation of all the nearby lines.

Running a formatter on every single commit is dumb, unless you're an enormous company that doesn't release their software (i.e. Google and very few other companies).

If you ship your software, make the autoformatter part of the release process. Commit the reformat (to the trunk) right before each branch-off. It's a great compromise. Everything is autoformatted "in the limit" but the disruptive commits happen only once per release cycle. Formatting-only PRs get rejected with "please wait until the next release and/or teach the autoformatter to do whatever it is you want here"

Right! What a mess of a commit

Look they used prettier on this css file how convenient

I like to bundle those in a separate commit at least.

I used to believe this but then I found myself spending way more time on clean single-purpose commits than I saved looking at a clean commit log.

Especially if the cleanup touches the same code you are semantically modifying you are basically forced into a bad dilemma of doing them in parallel (leading to a merge conflict with yourself yuck), or doing them in series which leads to the question of whether you should twiddle your thumbs while waiting for the first commit to get through review or whether you should do something else in the meantime risking forgetting the other commit.

That's why I don't think it's completely clear cut.

Reply on news.ycombinator.com