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

Metrics/BlockLength is overreaching #3772

Closed
NobodysNightmare opened this issue Dec 5, 2016 · 1 comment
Closed

Metrics/BlockLength is overreaching #3772

NobodysNightmare opened this issue Dec 5, 2016 · 1 comment

Comments

@NobodysNightmare
Copy link
Contributor

Context

The newly introduced Metrics/BlockLength filled a gap that existed in rubocop: Being able to write extra-ordinarily long blocks without a violation.

Now, by default, rake tasks and sinatra gets need to be within 25 lines to be okay.

However, since blocks are used in many different contexts, it seems to be harsh to limit all of them to 25 lines. E.g. I would currently disable the cop because it also affects my rails routes.
In the default settings it has already been disabled for RSpec directories.

Categorizing blocks

I can make up at least three categories of blocks:

  • control statement sized blocks (each, map, etc.)
  • method sized blocks (task, get, it, etc.)
  • class/module/namespace sized blocks (namespace, RSpec.describe, routes.draw)

IMO those need to be treated separately, otherwise people will simply disable the cop, because of too many "false positives".

Solution (?)

I had two ideas in mind how to solve that issue:

  1. whitelist some keywords to be allowed to violate the Max setting
    • i.e. not checking those at all
    • this is basically a "disable this cop for methods called X"
  2. have different limits/settings for different keywords?
    • this would allow more fine grained control
    • but I have no good idea how the configuration would look like

The idea behind both solutions is, that I don't need to disable the whole cop, but that I can keep it enabled for the important cases and disable/weaken it for the other cases.

What do you think? Are there other/better ideas on how to handle this?

@Drenmi
Copy link
Collaborator

Drenmi commented Dec 16, 2016

I personally think excluding entire files that are pure DSLs, like Rails routes.rb or RSpec spec files, and use the inline disabling for DSLs that go inside a regular class, like an aasm state machine declaration in a Rails model.

One problem with the proposed addition is that any number of classes can implement a method that takes a block, with the same name, creating room for a lot of false negatives.

NobodysNightmare added a commit to NobodysNightmare/rubocop that referenced this issue Dec 19, 2016
Changes default configuration to exclude methods, not files.

This allows to still enforce short it statements in RSpec
and short tasks in Rake, while not offending for contexts and
namespaces.
sihugh added a commit to alphagov/collections-publisher that referenced this issue Jan 31, 2018
This is a new addition and seems a little heavy handed.  It looks like there
will be [a refinement in future](rubocop/rubocop#3772)
to make this work a little better.
sihugh added a commit to alphagov/collections-publisher that referenced this issue Jan 31, 2018
This is a new addition and seems a little heavy handed.  It looks like there
will be [a refinement in future](rubocop/rubocop#3772)
to make this work a little better.
sihugh added a commit to alphagov/collections-publisher that referenced this issue Jan 31, 2018
This is a new addition and seems a little heavy handed.  It looks like there
will be [a refinement in future](rubocop/rubocop#3772)
to make this work a little better.
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

2 participants