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

Move `LineLength` cop from `Metrics` to `Layout` department #7542

Open
wants to merge 1 commit into
base: master
from

Conversation

@koic
Copy link
Member

koic commented Dec 1, 2019

Summary

The Layout department was created after Metrics was created.

As far as I use LineLength cop, I think this cop belongs to the layout, not the logic metrics. Rather than changing the logic when LineLength cop becomes a offense, a solution is often a line wrap.

The horizontal length targeted by LineLength cop is resolved by often a line wrapping, while the vertical length targeted by ClassLength cop, MethodLength cop, and BlockLength cop is
resolved by a logic change.
So ClassLength, MethodLength, and BlockLength cop remain in
the Metrics department.

Other Information

I'm wondering if the default maximum length of 80 is reasonable.
This is because there are cases where clear variable name (and etc) have to be omitted due to restrictions on LineLength.
However, this PR does not handle it because first of all this PR aims to move department.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
@koic koic force-pushed the koic:move_line_length_cop_to_layout_department branch from 58e5a8d to b56bcdf Dec 4, 2019
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Dec 4, 2019

I agree with your reasoning, but this might be confusing to the end users. @rubocop-hq/rubocop-core what do you think about the proposed change?

I'm wondering if the default maximum length of 80 is reasonable.

I guess we can revisit this before 1.0 when we discuss which defaults should be updated.

@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Dec 5, 2019

I tend to agree with the move, actually. 👍


As for the default length, I think 80 is the only length that has an actual justification. The problem is that justification is veeery outdated, dating back to IBM punch cards.

There does exist some research for optimal line length, but that is for natural language texts, and the results are inconsistent. I'm not aware of any such studies for code.

Bottom line is whatever is chosen is going to be arbitrary. 🙂

### Summary

The `Layout` department was created after `Metrics` was created.

- Create namespace Metrics and move some cops there
  e2153e8
- Layout department
  #4278

As far as I use `LineLength` cop, I think this cop belongs to the layout,
not the logic metrics. Rather than changing the logic when `LineLength`
cop becomes a offense, a solution is often a line wrap.

The horizontal length targeted by `LineLength` cop is resolved by
often a line wrapping, while the vertical length targeted by
`ClassLength` cop, `MethodLength` cop, and `BlockLength` cop is
resolved by a logic change.
So `ClassLength`, `MethodLength`, and `BlockLength` cop remain in
the `Metrics` department.

### Other Information

I'm wondering if the default maximum length of 80 is reasonable.
This is because there are cases where clear variable name (and etc)
have to be omitted due to restrictions on `LineLength`.
However, this PR does not handle it because first of all this PR aims
to move department.
@koic koic force-pushed the koic:move_line_length_cop_to_layout_department branch from b56bcdf to 6deb06b Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.