RadiusNetworks / radius-spec

Common Radius RSpec setup and plugins
Apache License 2.0
1 stars 1 forks source link

LineLength Max: 120? #3

Closed jnebeker closed 6 years ago

jnebeker commented 6 years ago

Just wanted to open this issue for discussion. Currently, the common rubocop configuration sticks to the default Metrics/LineLength constraint of 80 characters. Previously in donkeykong we had a higher limit of 120 characters:

Metrics/LineLength:
  Max: 120

I'm not sure how strongly people feel about this, I know that the style guide has the 80 character limit, and some other style guides suggest a 100 character limit.

Do we want to stick to the default 80 character limit or increase it to something like 100 or 120?


For context on how some of our apps currently align to the constraints, I ran rubocup on different apps with the 80 character limit and 120 character limit:

App 80 char. offenses 120 char. offenses
captain_crook 349 64
hamburglar 219 13
donkeykong 191 6
gossamer 427 24
kronos 444 76
kracken 246 22
sauron :scream: 0 ๐Ÿ‘ 0

After seeing what @cupakromer has done with sauron I'm not even sure I want to switch to the 120 limit anymore. Instead we could keep the default 80 character limit as a constant reminder to strive for greatness.

cupakromer commented 6 years ago

I extracted a few more settings from the project in #2. One of those was stuff around line limits. It sets it to 90 as a compromise. Generally prefer 80 but don't complain if you have to go a little over. I was debating about going to 85 to stress the "prefer 80" but thought maybe that was too restrictive at the moment.

It also makes some concessions for lines which are long due to comments.

Happy to get your thoughts on that PR.

csexton commented 6 years ago

I prefer the 120 (which is why I added it to donkeykong). This is personal preference from back in days of yore, but my take:

  1. It effects readability where we are forced to wrap lines just to make the cop happy
  2. Drives up line count because we are wrapping things more often, which will trigger the too-many-lines cop
  3. Discourages descriptive names

But, if we disagree then we fall back to the rubo-default.

cupakromer commented 6 years ago

IMO 120 is too long. Just to verify, I tested on my laptop and external monitor with a split code window. 120 wraps on both with just 2 window panes (b/c I also have line numbers, git gutter, etc.).

I thought Rubocop was smart enough to understand that a method with a multi-line call was "one-statement". I double checked and you're correct it's not ๐Ÿ˜ž so even just basic method param wrapping increases the method line count.

I applied the changes per #2 to test DK and CC to see how much the comment exceptions make a difference. This is what I found:

App 80 char 85 char 90 char 100 char 120 char
cc 299 250 208 132 69
dk 168 117 77 48 6

While I'd still personally prefer the 80 - 90 range, I'd be ok with setting it to 100. I can split that without a 2 window split forcing wrapping.

jnebeker commented 6 years ago

๐Ÿ’ฏThat sounds like a good compromise, we could maybe vote on 90 vs. 100 during today's DLL if we wanted.