On 2025-02-13 14:18, Stephan Bergmann wrote:
On 13.02.25 11:36, Chris Sherlock wrote:I see commits all the time where someone changes a line of code and changes the whitespacing in that same line of code.I occasionally see such too, and always wonder why people do that. My objections are twofold:For one, unnecessary and unrelated changes are distractions when looking at a commit. When a commit wants to do X, then it should do that, but not also randomly add or remove whitespace here and there, even in lines it touches for other reasons. There are cases where every little iota counts when you look at a commit, so do others a favor and keep your commits simple.And for another, what is the benefit of such random whitespace changes? The code is readable, before and after. Everybody has their preferred style rules, and we have to be able to cope with a mix of styles anyway. The goal should not be that the code is formatted according to the preferred style of programmer A (and then programmer B comes along and reformats lines according to their preferences)
While I'm aware the level to which people (dis)like clang-format (and the config we use for it) varies greatly, I personally usually (roughly) stick to its "taste" also in excluded files when introducing new code and to some extent when changing existing code (like whitespace in a line changed anyway, but not reformatting whole blocks of code in a way that would in my perspective more significantly clutter the commit diff and make it harder to see the "actual change").
Thinking about why I do that, it's probably because I saw others doing it earlier and it seems like a way to slowly move towards a code base that more consistently uses a "common" coding style (that is mandated by clang-format in other places), without changing unrelated lines (and thus having those show up in a `git blame` etc. while they wouldn't otherwise).
To me, that currently feels like a reasonable balance between 1) minimizing the commit diff 2) moving towards a more consistent coding style across the codebaseI see how this is motivated by myself considering 2) a desirable goal, while others may disagree.
Now I don't know whether it would make sense to try to get a consensus on that across contributors and then either consider that kind of change acceptable or discourage it "officially" (or whether it's better to leave it for everyone to decide what they do in their own commits and reviews)?
(Using a strategy that tries to stick to the coding style used elsewhere in the file for consistency when doing changes turned out not to be a workable approach in general as files often don't use a consistent coding style anyway. Using (git-)clang-format also seemed to be the "easy way" here without having to think about where exactly to break long lines in newly introduced code,...)
---the goal should rather be that the code is roughly readable and understandable. (To paraphrase an example from the original PragProg book: Mechanical workers also don't spend time making their power drills look beautiful. It's tools for them that need to get their job done in a professional way, but it's irrelevant that they may look patched and ugly.)
For me personally, "ugly code" generally tends to be less readable, and given the overall complexity of LO code sometimes adds to my confusion while debugging/reading code. For me personally, consistency ("principle of least surprise") helps reduce distraction and focus on the actual issue I'm looking into. (Now this arguably applies less for the exact placement of the space character in an if statement and more for things like extra/missing indentation of a block, misleading variable names (local variable `double mrMyInt = 42.0;`), and other creative things that can be done in LO source code.)
In the illustration of the power drill: As long as the power drill (the code) does what it should, I usually don't care what it looks like. Only when I have to look at it more closely to figure out why it doesn't work, the ugliness starts to bother/distract me.
That's my take on it. Of course, YMMV
Same here: The above is just my personal take, and if the majority considers "minimizing the commit diff" the guideline that should be followed, I'll adhere to that in the future.
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
