Rubocop: --auto-gen-config misses offenses when Metrics/LineLength Max is set

[garettarrowood]

Expected behavior

rubocop --auto-gen-config to create a todo file that catches all Style/WhileUntilModifier & Style/IfUnlessModifier violations. This is not the case when the todo file generates a Metrics/LineLength Max AND the Metrics/LineLength Max is set in the .rubocop.yml.

So with a .rubocop.yml file that contains:

Metrics/LineLength:
  Max: 100

And a foo.rb ruby file with this:

if some_conditional?
  one_line_of_code_that_is_100_chars
end

some_other_line_of_code_that_is_300_chars_long

I expect running rubocop --auto-gen-config to generate a todo with these items:

Metrics/LineLength:
  Max: 300

Style/IfUnlessModifier:
  Exclude:
    - 'foo.rb'

Then I expect to run rubocop and foo.rb to be green because the todo generated all the necessary exclusions.

Actual behavior

With the above .rubocop.yml & foo.rb, run rubocop --auto-gen-config. The actual todo file only includes Metrics/LineLength:

Metrics/LineLength:
  Max: 300

In other words, it misses the Style/IfUnlessModifier offense because correcting it would exceed the .rubocop.yml 100 char Metrics/LineLength Max. Although this example only demonstrates Style/IfUnlessModifier, the exact same is true for Style/WhileUntilModifier.

Running rubocop on foo.rb then produces an offense.

Steps to reproduce the problem

See above.

RuboCop version

0.52.1 (using Parser 2.4.0.2, running on ruby 2.3.3 x86_64-darwin15)

[jonas054]

The setting of Metrics/LineLength:Max in .rubocop.yml overrides the generated one in .rubocop_todo.yml. For this reason, the effective Max is 100, so the if statement in foo.rb cannot be changed to modifier form and there is no offense.

On the lastest master, we have the change #5371, which adds a warning on the console for these situations, when a parameter overrides another setting within the project.

[garettarrowood]

Ok. That makes sense. My big concern here is that one cannot generate a working todo file. This makes upgrading versions extra difficult. rubocop should always run green after generating a fresh todo. Can the regular rubocop command be extended to work the same way?

[jonas054]

It's the regular rubocop command that issues the warning:

.rubocop.yml: Metrics/LineLength:Max overrides the same parameter in .rubocop_todo.yml

The rubocop --auto-gen-config command outputs

Phase 1 of 2: run Metrics/LineLength cop (skipped because the default Metrics/LineLength:Max is overridden)

If we really wanted a watertight .rubocop_todo.yml we'd have to remove the LineLength:Max from .rubocop.yml in the process. That could be regarded as a bit heavy-handed.

I think the current solution is the best that we can do for the time being, without rethinking large parts of the configuration logic. In this special case we still have offenses after a rubocop --auto-gen-config, but before #4985 there were more cases like that.

[garettarrowood]

Thanks for these details, @jonas054 . I was unaware of any previous issues with the --auto-gen-config option.

I'm open to ideas to improve the process. Perhaps we should add a longer warning/explanation in the --auto-gen-config output? What's our recommended happy path in this scenario? Fix the offenses or use the default Max and then run --auto-gen-config again?

Related, but partially a new topic:

Do we have any docs with a recommended order to enable and autocorrect offenses in large applications? It wouldn't need to be elaborate, but it would be nice to have a central location to list things like enabling indentation/whitespace/layout cops before others, because using autocorrect with others might cause new offenses without them. This issue and workarounds could live on this document. And any other breaking scenarios or issues to watch out for I/we may not be aware of.

[jonas054]

What's our recommended happy path in this scenario?

If I interpret your situation correctly, my recommended path for this scenario would be to comment out the setting in .rubocop.yml:

# My goal is to have 100 as the maximum line length, but I'm not there yet, and while I
# work my way down towards 100, the current Max can be found in .rubocop_todo.yml.
# Metrics/LineLength:
#   Max: 100

Having said that, my overall recommendation is always to fix the offenses instead of disabling them! :smile:

[garettarrowood]

That's the approach I ended up going with. And although this is not a big concern, just to note, a byproduct of doing this is an inflated total offense count for LineLength:Max (because it's based on the standard 80 instead of 100)... This isn't worth sweating about at all though. Will prioritize resolving Style/IfUnlessModifier and Style/WhileUntilModifier offenses to avoid the problem moving forward.

[ClayShentrup]

+1 on fixing this. Very frustrating. I'm currently thinking of a way to add this to my todo.

[JeanMertz]

EDIT looking at this again, it's clear that my --exclude-limit 1000 was too low, since it recorded 1229 offences. After upping the exclude limit, things work as expected.

Perhaps this should be another enhancement, to somehow inform the user when this happens? Although I can understand that the output might become a bit chatty if we did that?

---

I just ran into this as well. Except, I ran Rubocop as follows:

rubocop --auto-gen-only-exclude --exclude-limit 1000 --auto-gen-config

I expected all files that violate the Style/LineLength Max value configured in the .rubocop.yml file, to be added to the list of Excluded files in .rubocop_todo.yml after running the above command, similar to what happens for all the other lints.

Instead, I get:

Phase 1 of 2: run Metrics/LineLength cop (skipped because the default Metrics/LineLength:Max is overridden)
Phase 2 of 2: run all cops

And the following is added to the todo config:

# Offense count: 1229
# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
# URISchemes: http, https
Metrics/LineLength:
  Max: 334

[jonas054]

Perhaps this should be another enhancement, to somehow inform the user when this happens? Although I can understand that the output might become a bit chatty if we did that?

Yes, I don't think we should warn the user that "maybe you meant to give a higher exclude limit". We don't know when that would be a useful warning so we'd have to give it always or never. I say never.

But the interaction between exclude limit and offense count seems off to me. The exclude limit should be the maximum number of files you want in an Exclude property, while the offense count is the total number of offenses, not the number of offending files. Do we have a bug here, or do you have one LineLength offense per file, @JeanMertz?

[JeanMertz]

Ah. No you’re right, there are definitely less than 1000 files on that exclude list.

[d0nt-panic]

@jonas054 Hi! Can you tell me please where I can find the explanation why this happens:

The setting of Metrics/LineLength:Max in .rubocop.yml overrides the generated one in .rubocop_todo.yml

And which cops are also prefer .rubocop.yml than .rubocop_todo.yml ? From my own experience I found that Metrics/BlockLength:Max has the same bevahior.

Thanks

[jonas054]

@d0nt-panic The description of how inheritance works in RuboCop is here: https://docs.rubocop.org/en/latest/configuration/#inheritance

But if you want a deeper understanding of the why behind the design decisions, I don't think you'll find it in the docs. So I'll try to give it here.

We wanted an inheritance mechanism that was easy to understand (principle of least surprise) and easy to implement, so it works like this:

1. Configuration files can inherit from other configuration files, and just like with inheritance in object-oriented languages, e.g. Ruby, the inheriter (subclass) can override settings in the inheritee (base class).
2. Ultimately every configuration inherits from the default configuration, but this is implicit, i.e. you don't need an inherit_from directive for the inheritance from default.yml.

It deserves mentioning that in #5371 we added a warning for when user config overrides other user config, but some people found the warnings annoying, so we removed them (except with --debug) in #5654.

P.S. There's no difference between cops. Settings in .rubocop.yml always override settings in .rubocop_todo.yml.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[stale[bot]]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.