Longer line length in code commits (100 vs 80)

I’m sure it’s a pretty contentious subject. But 80 characters is pretty short on a modern computer screen.
Even the tiny little edit box on this Discourse site allows for about 87 characters in width…

Couldn’t it get bumped up to 100 characters instead of 80?

As for binary, the next would be 96 and/or 128 :wink:

I was thinking more 100 to stay aligned with Linux source code guidance.
Preferred break at 80 characters, line limit of 100 characters.

The two lines below I think are equally comprehensible… but alas the second is 81 characters, and required a line split

        printfunc("Initializing SD Card (over SPI) and Mounting file system...")
        printfunc("Initializing SD Card (over SDIO) and Mounting file system...")

I tend to find that 80 columns promotes spending a lot of time on “code golf” :golf: and reformatting lines. Also the line limit promotes terse or unintelligible variable names.

This is especially true for parameter lists for making calls to the MCU. Also the markdown files for the help are all also locked to 80 characters. Long lines are hard to read, but so is 1 function call spread over 5 lines.

In my professional work no one uses 80 columns. Most teams opt for some kind of limit but it’s usually at least 120. Modern monitors and editors almost always have space for 120 columns.

Python PEP8 recommends 79 chars. That feels anachronistic.

3 Likes

Hi,

In brief, I don’t think we should change the line length tests in Klipper.

In general, I don’t think strict coding styles are good for large projects. For this reason, I did not want to introduce the 80 character test to the Klipper build checks. I do think, however, that it is important that each individual code file maintain a consistent internal style and that subsequent modifications maintain that coding style. So, for example, if the existing code uses 4 spaces for indent, future modifications should as well. Same for line lengths. If a new person takes over the maintenance of a file/files and wants a different style - then it makes sense to let that maintainer set a new style - but short of that I think the existing style should be followed.

Prior to introducing the line length check, a large number of PRs were proposing minor changes to existing code that did not follow the existing whitespace style (introducing tabs and/or using notably longer line lengths). Thus the build checks made it easier to communicate the need to maintain that portion of the existing code style with less back-and-forth in review comments.

If someone were contributing a large amount of code in a new subsystem and really wanted a different whitespace style, we could discuss altering the build checks for that new sub-system.

As to altering the style of the existing code to 80 vs 96 vs 120 and/or tabs vs spaces - I don’t think there is a “right or wrong” here. I prefer 80 and spaces. It’s also a pretty common standard in the industry. I don’t see a compelling reason to revise it now.

Hope that helps,
-Kevin

2 Likes

I actually very much sympathize with not wanting to deal with committers various style choices. I’ve done good things with teams where I added check style to the build, and made it easy for devs to format code in their IDE automatically. It really changes what happened in code reviews. No more “nitpicks”.

I got curious what would happen and I had ChatGPT write me a script to reformat klippy/* using the tool Black from 79 to 160 characters long. I had it count the total number of lines in the repo and draw me a graph:

black-line-lengths

Black claims that 88 is the magic number but your codebase seems to like ~100.

1 Like