code: relax max line length to 120

[smlng]

Contribution description

Relax when vera++ will error bc of line length, set max from 100 to 120 here.

Testing procedure

let CI run static checks

Issues/PRs references

• style-check/vera++: add style validator and scripts #9778 original PR introducing vera
• Add driver for SPS30 particulate matter sensor #13256 had issues bc of 100 chars length error

[smlng]

100 per coding conventions, I guess this would result in long discussion

[benpicco]

#13391 runs in the same issue (and vera++ is complaining about existing code that the PR touches)

[jia200x]

#13391 runs in the same issue (and vera++ is complaining about existing code that the PR touches)

Unfortunately Vera++ cannot run at patch level, so this kind of errors are expected.

I see some solutions to these:

1. Add some bash logic to make it run at patch level (Vera++ gives the line number, so it would be possible to match the lines modified by the patch and those given by the tool)
2. Piggyback these fixes (at least in C files, there shouldn't be that many cases with lines bigger than 100 characters)
3. Open the discussion of increasing the character limit to 120

[smlng]

I reopened this one to (re)start/continue discussion on the max line length in our coding conventions. I added commits that adapt the documents accordingly.

Why increase from 100 to 120?

First and most importantly: readability - its sometimes hard or infeasible to write good structured code with proper documentation and keep the 80 char limit, even 100 is challenging from time to time.

Second, reviewing blogs and other projects shows that this discussion is everywhere and an absolute maximum of 120 chars per line seems to be somewhat common ground among them.

Third, we already allow 120 (actually 119) for python code in RIOT (see flake8 config), so why not relax this for C/C++ code as well?

Forth, add your own argument

[MichelRottleuthner]

Looking at the numbers could help us decide. With the current setting of 100 chars we get 1853 affected lines. Relaxing it to 120 will still leave 272 lines open that would need to be addressed with either (1) or (2) as proposed by @jia200x.
(1) is probably not worth doing for that rather small number of lines and the time would IMO be better spend aligning the code with our existing coding conventions.

For me the 80 chars limit is more often painfully limiting than helpful. 120 on the other hand is already a bit much when considering a split pane with two files on a smallish (i.e. laptop) screen.

$ find . -iname "*.[c,C,h,H,cpp,hpp]" -not -path "*/vendor/*" | xargs -I X cat X | grep -c '.\{101\}'
1853
$ find . -iname "*.[c,C,h,H,cpp,hpp]" -not -path "*/vendor/*" | xargs -I X cat X | grep -c '.\{121\}'
272

[benpicco]

I think coding style rules should be descriptive rather than prescriptive, that is we shouldn't add/enforce rules that would require changing large parts of the codebase.

[smlng]

I think coding style rules should be descriptive rather than prescriptive, that is we shouldn't add/enforce rules that would require changing large parts of the codebase.

I don't understand this bc as @MichelRottleuthner pointed out there are already 1800+ lines violating the current rule of max 100 chars per line, from these less than 300 would remain when changing this to 120 chars max.

Currently these lines do not pop up in CI bc all (static) checks only run against changed files, hence if we do not touch or add files with 100/120+ chars per line, there are no errors. However, these should be fixed anyway.

[jia200x]

For me the 80 chars limit is more often painfully limiting than helpful. 120 on the other hand is already a bit much when considering a split pane with two files on a smallish (i.e. laptop) screen.

Vera++ is configured to show a warning if line length > 80. I think it's good to aim for that.
In cases where forcing 80 chars makes something hard to read, I don't think it would be that bad to change the coding convention to 120 chars. Less than 300 occurrences seems reasonable to be fixed on the fly.

[smlng]

120 on the other hand is already a bit much when considering a split pane with two files on a smallish (i.e. laptop) screen.

considering your own statistic its unlikely that you will open two files with lines exceeding 120 chars in RIOT 😉 Also, the aim is still 80 chars, but in some (rare) cases up to 120 chars should be allowed see for instance your own PR #13256 and the otherwise needed changes in saul.h, just because the Doxygen docu exceeds 100 chars by a bit.

[MichelRottleuthner]

considering your own statistic its unlikely that you will open two files with lines exceeding 120 chars

True, I was more thinking in the direction that explicitly allowing 120 chars could eventually increase the number of files where this is used.

All I wanted to say is that the number of affected lines is actually not horribly big even with the 100 chars limit and that the problem will not be completely solved by just pushing it to 120.

[smlng]

problem will not be completely solved by just pushing it to 120.

true that, so then we go full force and be strict on 80 chars 😩 just kiddin'

Anyway, that's what this is about: having an open discussion. And either outcome --change to 120 or stick to 100 chars max-- is fine.

[gschorcht]

I wish we had three levels: info > 80, warning > 100 and error > 120.

In my opinion, in times with large high-resolution displays, the 80 characters shouldn't be a dogma, even though they should be the normal case. Very often line breaks with 80 characters make the code unnecessarily unreadable. There are special cases where also 100 characters are not enough, for example, if URLs are used as references in comments.

[smlng]

the "new" argument nowadays for 80 chars is that you can open 2 files side-by-side on a high-res (laptop) screen without line breaks or wraps, 120 is too much then again.

Anyhow, I'm in favour of being less strict with 80 and even 100 chars, hence this PR.

[tcschmidt]

@kaspar030 can you provide comments on this HCI issue?

[kaspar030]

@kaspar030 can you provide comments on this HCI issue?

I guess you mean CI?

CI could provide the set of changed lines per changed file, or the new lines. Quick googling shows that would be possible: https://gist.github.com/mdawaffe/529e6b3ee820e777c2cfd2f8255d187f

We need to converge on line length rules first. At that point we should fix the codebase, turning line-based checking obsolete, so I don't think we should waste time in that direction.

[benpicco]

At that point we should fix the codebase

That sounds like a lot of effort with little benefit.

[kaspar030]

That sounds like a lot of effort with little benefit.

Depends. I'm still for enforcing uncrustify on everything, just to get rid of these discussions. I think the possibly weird formatting in corner cases (that don't suggest excluding from formatting) are far less of a problem than the wasted time on "CI says line X in file Y is too long" etc.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.