Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LineLength Max: 120? #3

Closed
jnebeker opened this issue May 15, 2018 · 4 comments
Closed

LineLength Max: 120? #3

jnebeker opened this issue May 15, 2018 · 4 comments

Comments

@jnebeker
Copy link
Contributor

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 😱 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
Copy link
Contributor

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
Copy link
Member

csexton commented May 16, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants