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

[Fix #3566] Add new `Metrics/BlockLength` cop. #3587

Merged
merged 2 commits into from Oct 8, 2016

Conversation

Projects
None yet
3 participants
@savef
Contributor

savef commented Oct 8, 2016

This cop checks there aren't too many lines in blocks, much like Metric/MethodLength checks for too many lines in methods.

By default the max lines has been set to 25 which seems a decent starting point as the Rubocop library passes without having to change our config file (but only due to the default excluded files).

The cop is excluded from checking the Rakefile, all .rake files, and all Ruby files in spec by default. All of these are pretty much intended to be large blocks, the length of classes or modules, and will easily breach a lowish limit.

I've left this as 2 separate commits, the first of which adds the cop, and the second of which refactors shared parts into a mixin. I'm not sure if there's a nicer way to make each cop's message work without having to have a method in each cop to pass its label through, like this.


Before submitting the PR make sure the following are checked:

  • Wrote [good commit messages][1].
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 8, 2016

Collaborator

The new cop looks good. Just drop those trailing . from your commit message titles.

Collaborator

bbatsov commented Oct 8, 2016

The new cop looks good. Just drop those trailing . from your commit message titles.

savef added some commits Oct 7, 2016

[Fix #3566] Add new `Metrics/BlockLength` cop
This cop checks there aren't too many lines in blocks, much like `Metric/MethodLength` checks for too many lines in methods.

Also removed the splatted additional args from CodeLength#check_code_length, I don't think they've been necessary since the old `check` method was refactored into that method.
Refactor shared `Style/MethodLength` and `BlockLength` code into mixin
The `CodeLength` inclusion, `#message` method, and `#code_length` are shared between both cops so have been moved into a mixin included by both.

Also added two test lines that ensure the messages are being set properly.

@bbatsov bbatsov merged commit 3c01f69 into rubocop-hq:master Oct 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonas054

This comment has been minimized.

Show comment
Hide comment
@jonas054

jonas054 Oct 8, 2016

Collaborator

By default the max lines has been set to 25 which seems a decent starting point as the Rubocop library passes without having to change our config file

I'd prefer a default of 10 - same as Metrics/MethodLength - and 24 in .rubocop_todo.yml. Allowing longer blocks than methods doesn't make sense to me.

Other than that, I love the new cop!

Collaborator

jonas054 commented Oct 8, 2016

By default the max lines has been set to 25 which seems a decent starting point as the Rubocop library passes without having to change our config file

I'd prefer a default of 10 - same as Metrics/MethodLength - and 24 in .rubocop_todo.yml. Allowing longer blocks than methods doesn't make sense to me.

Other than that, I love the new cop!

@savef

This comment has been minimized.

Show comment
Hide comment
@savef

savef Oct 8, 2016

Contributor

I started off thinking 10 was a good idea too until I saw how many places it failed. 😋

Admittedly excluding it on specs and rake things helped, but there's still quite a lot. I imagine every Rails application's config/routes.db file has blocks longer than 10 lines, for example.

Perhaps we should put this in a new issue so more people see it and can voice opinions? I imagine not many will in here.

Contributor

savef commented Oct 8, 2016

I started off thinking 10 was a good idea too until I saw how many places it failed. 😋

Admittedly excluding it on specs and rake things helped, but there's still quite a lot. I imagine every Rails application's config/routes.db file has blocks longer than 10 lines, for example.

Perhaps we should put this in a new issue so more people see it and can voice opinions? I imagine not many will in here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment