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

Introduce new cops with a special status (instead of enabled/disabled) #5979

Closed
bbatsov opened this issue Jun 9, 2018 · 13 comments · Fixed by #7567
Closed

Introduce new cops with a special status (instead of enabled/disabled) #5979

bbatsov opened this issue Jun 9, 2018 · 13 comments · Fixed by #7567
Assignees
Labels
enhancement high priority A ticket considered important by RuboCop's Core Team
Milestone

Comments

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 9, 2018

We have to add a way to mark recently added cops as new and have the users decide themselves whether they want to enable them or not.

I think that we can have Enabled feature a new value (e.g. New or nil) and cops marked this way will just produce a message on RuboCop start saying something like:

The following cops were added and you need to decided whether to enable or disable them.

This is going to solve the commonly reported problem that RuboCop updates are very painful for end users as usually they break their builds due to new cops being added. This would allow people go just bugfixes without having to mess much with the config. Going forward we'll take a page from ESLint and enable/disable cops upstream only in major releases.

This ticket is somewhat related to #5978, as we'd use the version data in the message we print to the users.

@bbatsov bbatsov added this to the 1.0.0 milestone Jun 9, 2018
@mikegee
Copy link
Contributor

mikegee commented Jun 11, 2018

This seems a bit duplicative with the VersionAdded and VersionChanged metadata. Do you think we can infer "newness" from versions?

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jun 12, 2018

@mikegee We can infer the cop status, but we still have to set it to something different than enable or disabled IMO. I don't like the idea of having cops set as disabled by default, because than you can't differentiate if you disabled something on purpose or it's disabled simply because it's new. What exactly is going to be the Enabled state for cops introduced in 1.1, 1.2 and so on if we don't add something for this?

@mikegee
Copy link
Contributor

mikegee commented Jun 12, 2018

I hadn’t thought it through as much as you. That makes sense. 👍

@Vasfed
Copy link
Contributor

Vasfed commented Jun 29, 2018

I think we can have something like 'LockVersion' in config - and then cops from a newer non-permitted versions can switch into non-error mode, or even just become disabled and only produce a warning that new cops were added and user should consider raising locked version.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jul 21, 2018

@Vasfed Perhaps. Generally all of this is trivial once someone has the down the ground work in #5978.

@bbatsov bbatsov added the high priority A ticket considered important by RuboCop's Core Team label Sep 12, 2018
@bbatsov bbatsov mentioned this issue Sep 26, 2018
14 tasks
@bbatsov
Copy link
Collaborator Author

bbatsov commented Sep 27, 2018

@Darhazer I guess it makes sense for you to reap the benefits of your work on the huge metadata tasks, so I'm assigning this to you.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Apr 27, 2019

@Darhazer would you be able to tackle this anytime soon or should we find someone else from @rubocop-hq/rubocop-core to work on it? It's one of the most important tasks on the way to RuboCop 1.0 and since it's not a big task it'd be nice if we wrapped it up in the next couple of releases.

@Darhazer
Copy link
Member

Sure. I'll do it in the next couple of weeks.
What about the wording? Something along the lines of

The following cops were added to RuboCop, but not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` config.

Also, since it would require copy/pasting cop names to the .rubocop.yml, perhaps a rake task to enable/disable a cop /all non-configured cops/ would be useful?

@bbatsov
Copy link
Collaborator Author

bbatsov commented May 22, 2019

@Darhazer The wording sounds great.

Also, since it would require copy/pasting cop names to the .rubocop.yml, perhaps a rake task to enable/disable a cop /all non-configured cops/ would be useful?

Great idea! This might also be some command-line option, but I rake task would be enough.

@Darhazer
Copy link
Member

Darhazer commented Jun 5, 2019

@rubocop-hq/rubocop-core I won't be able to complete this at least two more weeks, so if someone wants to take it, feel free to do so

@pirj
Copy link
Member

pirj commented Aug 25, 2019

Elaborating on @Vasfed's idea.

Imagine an acknowledged_version setting:

require:
  - rubocop/cop/internal_affairs
  - rubocop-performance
  - rubocop-rspec

acknowledged_version:
  rubocop: 0.72.0
  rubocop-performance: 0.60
  rubocop-rspec: 1.34.1

Imagine we set Enabled: true for all cops.
When the user updates RuboCop or extension version, when we run rubocop, we have information about the original version and the destination version.
Thus we know the "diff", e.g. which new cops were introduced between those two versions. E.g.:

There are some newly added cops that are not acknowledged.
The following cops were added to RuboCop.
Run `rubocop --auto-gen-config` to update your `.rubocop_todo.yml`, and optionally disable them completely if they don't match your project's established style.

RuboCop: 0.72.0 -> 0.74.0
  Style/MultilineWhenThen
RuboCop/Performance: 0.60.0 -> 1.4.1
  Performance/OpenStruct 
RuboCop/RSpec: 1.34.1 -> 1.35.0
  RSpec/ImplicitBlockExpectation http://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ImplicitBlockExpectation

Benefits:

  1. no need to replicate the whole list of cops in their configs, only disable the cops.
  2. less effort, it's possible to rubocop --auto-gen-config to disable the new cops that have offences. Users might decide to disable them completely or leave in todo.

@bbatsov @Darhazer @mikegee WDYT?

@pirj
Copy link
Member

pirj commented Aug 25, 2019

Please disregard the above comment, I've completely missed the discussion in the related pull request and this comment.

@tas50
Copy link
Contributor

tas50 commented Dec 6, 2019

In Cookstyle, which is a wrapper for RuboCop, we've worked around this issue by setting the default fail level to convention and introducing all our new cops at refactor level. That way we can introduce new cops that will show up on the CLI and autocorrect, but won't fail CI builds. That allows our users to always run the latest version of our tool without constantly spending time updating their codebases when new versions are released.

bbatsov pushed a commit that referenced this issue Dec 24, 2019
Previously, new cops were introduced as either enabled or disabled. The
ones enabled were bothering users with new offences, while disabled cops
were often left out and remained under their radars, while still being useful.

By introducing this special status, users have to decide how to handle
new cops, by explicitly enabling or disabling them.

Cops are to be introduced with pending status between major releases of
RuboCop and its extensions, and they eventually become enabled or
disabled on major releases.

Co-authored-by: Phil Pirozhkov <hello@fili.pp.ru>
koic added a commit to koic/rubocop that referenced this issue Feb 5, 2020
Follow up of rubocop#5979.

This PR shows pending status in cop status of manual.
The pending status is not enabled status and may lead to misreading.
bbatsov pushed a commit that referenced this issue Feb 5, 2020
Follow up of #5979.

This PR shows pending status in cop status of manual.
The pending status is not enabled status and may lead to misreading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement high priority A ticket considered important by RuboCop's Core Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants