Add new EnableNewCopsUpTo option to enable all pending cops up to a specific Rubocop version

[zofrex]

I would like to modestly propose a new feature for enabling pending cops in bulk:

I realised while creating my .rubocop.yml for a new project, that what I wanted to do with pending cops wasn't to enable all of them (and potentially fail CI in the future due to new cops) but nor did I want to ignore pending cops available right now that I could ensure my code passes. I know I could add each one individually to .rubocop.yml but that's a bit tedious and my configuration is quite short and I'd rather keep it that way.

What occurred to me is that I'd like to enable all the cops that were available in the version of Rubocop I'm using, and not ones in newer versions. And thus this feature idea was born.

With this feature you can set the EnableNewCopsUpTo option in your .rubocop.yml file:

AllCops:
  EnableNewCopsUpTo: '0.89'

And all pending cops that were added in that version or earlier will be enabled, and any added in the future will give a warning just like they do presently.

When you see those warnings and have some time, you can try enabling them, fix any issues, and bump the version number in your config again.

I'm not married to any of the code here, but I'd rather bring a working demo along when proposing a feature. The version comparison code is maybe not great, it was just the fastest way to get a semver comparison to happen. I'm even less married to the option name which is quite clunky.

I added tests for the change in registry.rb but didn't really grok how to test the changes in config.rb.

I also added a first pass at documentation, but it could probably be clearer and will of course need to change if any of the names change.

I'm open to suggestions and feedback on any of the changes here! I thought this might be a neat way to deal with the problem and to have my cake and eat it, and based on reading some threads around here I have a feeling I'm not the only person who wants new cops but doesn't want a long list of enabled pending ones in their .rubocop.yml. Let me know what you think!

---

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). (N/A)
• 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.

[bbatsov]

I'm afraid the proposed solution doesn't work with extensions (as they also introduce cops as pending, but have different version numbers), which is why such a feature request was rejected originally. See this ticket for more details #8249

[zofrex]

Ah I see, I missed that thread – that's my bad, sorry!

I'd like to still discuss this a little more if that's ok, although if you consider this already discussed and decided and don't want to re-discuss it I understand :)

Firstly I'll happily admit I don't know much about extensions, I haven't used them much, so I might be wrong on any point here and would be grateful for anyone with more experience chiming in!

However my gut feeling is that, from a user perspective, even if you have to separately specify versions for each extension this would still be less work? I'm guessing that most extensions have more than one cop, so it would be fewer lines of configuration to enable pending cops via version than by writing each one out.

Here's a real-world example from my current project of what the .rubocop.yml looks like if I enable each pending cop from core and rubocop-minitest, vs what it might look like with a version option for each extension:

Add each pending cop (current scenario):

require: rubocop-minitest

inherit_from: .rubocop_todo.yml

AllCops:
  TargetRubyVersion: 2.5

# I like parallel assignment.
Style/ParallelAssignment:
  Enabled: false

# imho
Layout/SpaceAroundEqualsInParameterDefault:
  EnforcedStyle: no_space

# Don't enforce block brace spaces in tests
# because _{foo} is common there and matches _(foo), _ { foo } looks weird because it doesn't match _(foo)
Layout/SpaceInsideBlockBraces:
  Exclude:
    - 'test/**'
Layout/SpaceBeforeBlockBraces:
  Exclude:
    - 'test/**'

# Not a relevant metric for tests, where we organise tests by block and those blocks get pretty large
Metrics/BlockLength:
  Exclude:
    - 'test/**'

Layout/EmptyLinesAroundAttributeAccessor:
  Enabled: true

Layout/SpaceAroundMethodCallOperator:
  Enabled: true

Lint/BinaryOperatorWithIdenticalOperands:
  Enabled: true

Lint/DeprecatedOpenSSLConstant:
  Enabled: true

Lint/DuplicateElsifCondition:
  Enabled: true

Lint/DuplicateRescueException:
  Enabled: true

Lint/EmptyConditionalBody:
  Enabled: true

Lint/FloatComparison:
  Enabled: true

Lint/MissingSuper:
  Enabled: true

Lint/MixedRegexpCaptureTypes:
  Enabled: true

Lint/OutOfRangeRegexpRef:
  Enabled: true

Lint/RaiseException:
  Enabled: true

Lint/SelfAssignment:
  Enabled: true

Lint/StructNewOverride:
  Enabled: true

Lint/TopLevelReturnWithArgument:
  Enabled: true

Lint/UnreachableLoop:
  Enabled: true

Style/AccessorGrouping:
  Enabled: true

Style/ArrayCoercion:
  Enabled: true

Style/BisectedAttrAccessor:
  Enabled: true

Style/CaseLikeIf:
  Enabled: true

Style/ExplicitBlockArgument:
  Enabled: true

Style/ExponentialNotation:
  Enabled: true

Style/GlobalStdStream:
  Enabled: true

Style/HashAsLastArrayItem:
  Enabled: true

Style/HashEachMethods:
  Enabled: true

Style/HashLikeCase:
  Enabled: true

Style/HashTransformKeys:
  Enabled: true

Style/HashTransformValues:
  Enabled: true

Style/OptionalBooleanParameter:
  Enabled: true

Style/RedundantAssignment:
  Enabled: true

Style/RedundantFetchBlock:
  Enabled: true

Style/RedundantFileExtensionInRequire:
  Enabled: true

Style/RedundantRegexpCharacterClass:
  Enabled: true

Style/RedundantRegexpEscape:
  Enabled: true

Style/SingleArgumentDig:
  Enabled: true

Style/SlicingWithRange:
  Enabled: true

Style/StringConcatenation:
  Enabled: true

Minitest/AssertInDelta:
  Enabled: true

Minitest/AssertionInLifecycleHook:
  Enabled: true

Minitest/AssertKindOf:
  Enabled: true

Minitest/AssertOutput:
  Enabled: true

Minitest/AssertPathExists:
  Enabled: true

Minitest/AssertSilent:
  Enabled: true

Minitest/LiteralAsActualArgument:
  Enabled: true

Minitest/MultipleAssertions:
  Enabled: true

Minitest/RefuteInDelta:
  Enabled: true

Minitest/RefuteKindOf:
  Enabled: true

Minitest/RefutePathExists:
  Enabled: true

Minitest/TestMethodName:
  Enabled: true

Minitest/UnspecifiedException:
  Enabled: true

Version specifiers for each extension (proposed solution):

require: rubocop-minitest

inherit_from: .rubocop_todo.yml

AllCops:
  TargetRubyVersion: 2.5
  EnableNewCopsUpTo:
    Core: '0.80'
    Minitest: '0.10'

# I like parallel assignment.
Style/ParallelAssignment:
  Enabled: false

# imho
Layout/SpaceAroundEqualsInParameterDefault:
  EnforcedStyle: no_space

# Don't enforce block brace spaces in tests
# because _{foo} is common there and matches _(foo), _ { foo } looks weird because it doesn't match _(foo)
Layout/SpaceInsideBlockBraces:
  Exclude:
    - 'test/**'
Layout/SpaceBeforeBlockBraces:
  Exclude:
    - 'test/**'

# Not a relevant metric for tests, where we organise tests by block and those blocks get pretty large
Metrics/BlockLength:
  Exclude:
    - 'test/**'

Before we get into the finer details of how such a feature might work technically I'd like to ask you what you think of this at a high level. Are my assumptions about extensions correct? Would this be less work for users? Is such an approach even feasible, given how extensions and versioning them works?

[bbatsov]

AllCops:
  TargetRubyVersion: 2.5
  EnableNewCopsUpTo:
    Core: '0.80'
    Minitest: '0.10'

I'm not fond of introducing a new configuration option, besides NewCops, as I think that adds unnecessary complexity to the config. I guess it'd be ideal if we retain the existing behaviour of the config unless it has been provided a map instead of a string or something along those lines. But the real problem is that there's no way to currently map cops to some extensions, other than checking their filenames and inferring it from there, which seems quite painful to me. It seems to me it'd be much easier to have the config list departments instead of plugin names, as that's easily available information and each cop has the metadata for when it was introduced. Most extensions have a single department anyways.

[zofrex]

That's fair, we can stick with just NewCops.

I took a shot at coding up the same thing but with NewCops pulling double-duty as specifying the version, and doing it per-department. The code isn't exactly production ready, but it works, which proves the concept. Here's my configuration file now, as an example:

AllCops:
  TargetRubyVersion: 2.5

Lint:
  NewCops: '0.89'

Style:
  NewCops: '0.89'

Layout:
  NewCops: '0.89'

Minitest:
  NewCops: '0.10'

Performance:
  NewCops: '1.7'

I mostly like this. It still solves the problem, this is a lot less to keep up to date than a long list of enabled cops, and it doesn't matter that the gems aren't in sync on version numbers as they're all controlled separately.

The only bit I'm not so keen on is having separate configuration for Lint, Style, and Layout. I suppose someone might want to control those separately, but often I think you'll want the same version on all of them, as they're all coming from the same place.

Is there a way to group those into one that makes sense? I'm worried about introducing any new concepts into the configuration, and that it might be confusing if most keys refer strictly to departments and one is referring to the gem the cops are from.

[zofrex]

I also want to mention that I've thought of some other potential solutions to this problem too. I'm not attached to solving the problem in the way I proposed above, I'm only attached to improving the ergonomics around pending cops :) So, I think it's maybe worth considering other approaches as well as refining the version-based solution?

Option 2: Common version number between extensions

Obviously extensions can't be kept strictly in sync version wise, but they could all have some shared version that is only bumped once in a while (yearly? quarterly?) e.g. "Version 2019" or similar. Like Rust's "epochs", a little.

Upsides:

• More feasible to keep in sync between extensions than a strict version
• Only a single configuration option to bump for all extensions

Downsides:

• Can't enable the latest pending cops until the version is bumped
• Requires a lot of co-ordination between extension authors

Option 3: Date-based enabling

Cops could have additional metadata for the date they were introduced, similar to the metadata they already have for which version they were introduced in. Users could then enable all cops introduced by a certain date.

Upsides:

• Still only a single configuration option to bump for all extensions
• Doesn't require any coordination between extension authors (everyone knows what date it is)

Downsides:

• Requires extra metadata being added to every new cop, by every extension author
• Might get a little out of sync if you pick a very recent date, as not everyone is on the same date due to timezones

Option 4: Make the current status quo less work

Adding every pending cop to your configuration file has two problems, as I see it: extra noise in the configuration file, and tedious work to add each one. I've found a nice solution to both problems in the meantime while working on this PR –

I've separated the pending cops part of configuration into its own file, and written a small Rake task that hooks into Rubocop and appends a line for each pending cop to that file. Now it's very easy to keep up to date!

Upsides:

• No added complexity to the core of Rubocop at all
• Works quite similarly to an existing feature, the --auto-gen-config option. Familiar to users!

Downsides:

• The configuration is less declarative - it's not obvious from reading it that the intent is to enable all pending cops that were available at the time
• Still requires dozens or even hundreds of lines of configuration, albeit no longer written manually

Personally, I still like the idea of being able to set version numbers for NewCops in configuration, I think it's the cleanest option. But as I said, I'm not attached to that, only to solving the problem. If you think one of these other approaches would be a better fit for Rubocop (or have other ideas entirely of how to do this), I'm game for trying to implement that instead :)

[bbatsov]

@rubocop-hq/rubocop-core Any thoughts?

For the protocol - I still like the most the idea to use a version per department/extension.

[marcandre]

From this issue and others (comes to my mind: #8490 and showing doc URL for example), it seems like there is a need to map a Cop class to a Corps (or Service?) which could provide meta information: unique identifier (like :builtin, :performance, :rspec), current version, map a cop class to a doc URL).

This could be done either via singleton class methods of Cop::Base, or with a Cop::Base.service returning a Corps object responding to those.

I wish I had a more informed opinion.

[koic]

For the protocol - I still like the most the idea to use a version per department/extension.

This approach looks good to me. It seems that setting NewCops for each department will solve the version difference for each extension cop gem. Users can ride based on the existing NewCops convention.

[bbatsov]

I guess we'll pick up this conversation again after we cut 1.0.

[zofrex]

It seems like everyone is happy with the first approach, version per department/extension?

If so I'm happy going ahead and turning my proof of concept into a decent patch.

[bbatsov]

@zofrex Go for it!

[bbatsov]

@zofrex Any progress on this front?

[zofrex]

@bbatsov I haven't had any time to spend on this since writing up the proof of concept, I'm afraid. I'm hoping to get to this in the next week or two, but if someone else wants to take it on then by all means go ahead, as I can't promise anything.

I'll push the proof of concept in case it's useful for implementing this for reals, but I don't think the code is in great shape.

By the way, I have noticed that it seems the logic for handling cops is duplicated: once for displaying pending cops on the CLI, and once for actually picking which cops run. I think with this amount of complexity being added to pending/enabled logic it would make sense to find a way to combine those, to avoid any disagreement between them which would be very confusing for users. I'm not sure how best to do that, though.

[bbatsov]

No rush. I was just going over all open PRs today.

[zofrex]

Ok, cool.

Do you have any thoughts on the duplication of the pending/enabled cops logic? Do you agree it's a problem, first of all? And if you do, do you have any thoughts on how to resolve it?

[pirj]

@zofrex There's not much logic in how Pending cops are treated.
If you happen to run in a situation where you would feel that extracting some parts would worth it - go for it!

By the way, WDYT about only allowing EnableNewCopsUpTo to be set to the same major version as RuboCop/extension version? E.g. for RuboCop 2.3.0, you can't set RuboCop/EnableNewCopsUpTo: 1.91, but you can specify 2.10.
Anyway, this feature is to adopt cops that are still Pending, before they are turned on on a major version bump, right? I.e. cops between 1.91 and 2.0 were enabled/disabled already, and are not Pending anymore.

[zofrex]

I've written a first draft of doing this as agreed, per department/extension.

It's missing one thing for sure, which is configuration validation – it wasn't clear to me how to add that for a per-department attribute. I'm sure there will be other things too!

By the way, WDYT about only allowing EnableNewCopsUpTo to be set to the same major version as RuboCop/extension version? E.g. for RuboCop 2.3.0, you can't set RuboCop/EnableNewCopsUpTo: 1.91, but you can specify 2.10.
Anyway, this feature is to adopt cops that are still Pending, before they are turned on on a major version bump, right? I.e. cops between 1.91 and 2.0 were enabled/disabled already, and are not Pending anymore.

@pirj this sounds like a good idea to me, what do you think the failure mode should be for this? A warning or an error?

[bbatsov]

Seems we all missed the last update on this PR. We'll review it shortly.

[zofrex]

No worries, I was going to resolve the merge conflicts before bumping :) I'll do that now!

[zofrex]

Updating these tests for Rubocop 1.0+ highlighted how fragile they are:

• They rely on the actual Rubocop version and will need to be changed when it passes the versions hardcoded here
• They rely on very specific output formatting from the CLI
• If anything updates underneath them (or a mistake is made when updating the tests), the "expect not_to include" ones have a tendency to pass, even if they shouldn't be, or aren't testing anything useful

[dvandersluis]

They rely on the actual Rubocop version and will need to be changed when it passes the versions hardcoded here

You probably could stub out the version constant for the tests.

[dvandersluis]

I agree though that these tests are brittle, it’d be good if we could generate a data structure of cops that will be outputted and use that both to output and to test against, rather than testing the output itself.

At the very least, instead of expect not_to include I would test for the specific full message so that it’ll fail if something changes, but this might get overly verbose.

[dvandersluis]

I’m not sure I love a per department setting because unlike the extensions which generally only have one, rubocop itself has a bunch of departments that would have to be configured separately and maintained. @marcandre’s “corps” idea might be a solution.

I’m also wondering if there might be different approach where we keep track of the last version of each gem that was “updated” to (there’d have to be a mechanism for tracking this), like a .rubocop-versions file or something, but this is just a rough idea. I want to make sure we get this right the first time because once it’s out there it’ll be way more complicated to revise.

Anyways, I haven’t looked at the code in depth yet but here are some edge cases to think of, how do we expect each of these behave?

AllCops:
  NewCops: 1.15

Lint:
  NewCops: 1.19

AllCops:
  NewCops: disable

Lint:
  NewCops: 1.19

AllCops:
  NewCops: 1.19

Lint:
  NewCops: disable 

Lint: 
  NewCops: 1.10

Lint/Foo:
  Enabled: pending

(We probably should have an error if someone tries to mark a cop as pending in their own config, unrelated to this PR specifically)

Lint: 
  NewCops: 2.0 # current version is 1.19

Lint: 
  NewCops: 0.0 # current version is 1.19

[pirj]

I believe "extension" is a more appropriate term.

[pirj]

I believe that extensions and departments are orthogonal. An extension can add cops to a department that exists in the core RuboCop.
Making this as flexible as allowing to configure it per-extension and per department is an overkill. AllCops/NewCops for RuboCop core and optional separate per-extension is just fine.

WDYT:

AllCops:
  NewCops: 1.15

equivalent to:

AllCops:
  NewCops:
    rubocop: 1.15

For extensions:

AllCops:
  NewCops:
    rubocop: 1.15
    rubocop-rails: 2.5

[pirj]

Setting a version number, is ambiguous.
Should a version of RuboCop core or an extension that introduced a certain cop to the existing department be used to decide if the cop is affirmed or not?

I believe some kind of warning should be issued in case of such an ambiguity, e.g.:

`Style/NewCops` is set to `1.15`. However, both `rubocop` and `rubocop-nitpicks` provide cops to the `Style` department, and their versions do not align.

[dvandersluis]

I think a problem will be we don’t know what extension adds what cop but I agree per extension makes more sense to me than per dept.

[dvandersluis]

@pirj mentioned issues with --enable-pending-cops on #8249, was that covered here?

[pirj]

Briefly skimmed through comments, and I think we've agreed that the approach with setting the version separately for extensions (and core) is the only choice.
I have no awareness of a technical feasibility, if the information which extension the cop comes from is available when we compile the list of pending cops to enable.

[pirj]

@zofrex Ping

[zofrex]

@pirj I like your proposed syntax for doing it by extensions, and the short-hand for the core cops. If other people are OK with that I'd be up for giving that a shot, although I think the prevailing assumption was that we don't have enough data for this currently?

I think the next step would be to look into how readily available that data is. I'll try to get to that, but don't let this vague promise stop anyone else from taking a look.

[pirj]

Ping @rubocop/rubocop-core