Conversation
|
I'm ok with this |
|
I'm awesome with this. Though there might be a logical reason to have some limit. EDIT One such logical limit might be how long lines work in github reviews. |
|
Thanks for the feedback @hamishwillee @mrpollo. Doesn't seem like anyone else has any feedback. Approve and merge? |
|
@MaEtUgR Do you care about this one? |
|
IMO, the limit is a good thing. I have seen people try to be "clever" with one-liners that go beyond the current limit. This is a good way to stop that behavior; however, it's also a bit restrictive for everyone else. Why don't we bump to a higher character limit instead? |
|
IMO the occasional long line is better than crap like this. There are also lots examples of one liners that are long by necessity (think class constructor inheritance with templates). My preference is to not chop up code on arbitrary boundaries like line length. I do agree that some people may try to be clever with one-liners that are too long and it would be good to prevent that, but in this case the cure is worse than the disease. |
|
I'm in favor of keeping a line length limit, am open to increase it to 140. |
6aeea8f to
5187fa5
Compare
MaEtUgR
left a comment
There was a problem hiding this comment.
Make sense to have a limit and increase it.
* astyle: remove max line length (#25717) * failsafe unit test: add cases for 1 allow taking over from degraded failsafes 2 not cause immediate takeover when failsafe happens because of mode switch The first test makes sure the user can take over when an RTL failsafe was triggered but degraded to a Land. The second test rules out the easiest fix of removing the condition `_selected_action == selected_action` which causes the problem for test one but is there for a reason. * commander/failsafe: fix user takeover not possible in fallback Land action when configured RTL is not possible --------- Co-authored-by: Jacob Dahl <37091262+dakejahl@users.noreply.github.com>
Opinion
Enforcing line length hurts readability by indenting logical one liners. IMO there isn't much point, I think most people have big monitors and would rather horizontally scroll than having broken up code scattered throughout.
Alternative
Bump to 140
Example
It's especially bad with one-liners that also contain a long comment on the same line
https://github.com/PX4/PX4-Autopilot/blob/main/src/modules/ekf2/EKF2.hpp#L506-L507