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

[WIP] [Fix #5979] Introduce cops with special status #7184

Closed
wants to merge 1 commit into from

Conversation

Darhazer
Copy link
Member

Consider this a proof of concept.

The approach I took creates a couple of problems I did not foresee.

First, the need to disable the warning in 'cli spec behavior' context, as otherwise, we are polluting the output with each new cop added.

The other issue is that the newly introduced cops would not be applied to the rubocop codebase as well unless we introduce an option to override the new status (e.g. enable all non-configured cops)

Let me know what you think and I can proceed with adding rake tasks for enabling new cops and any other supporting options.


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).
  • 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.

@Darhazer Darhazer self-assigned this Jun 27, 2019
@Darhazer
Copy link
Member Author

Perhaps a better approach would be to have a VersionLocked in the user's .rubocop.yml and for cops introduced in later versions - to check if there is Enabled in the user's config, and if no - warn the user about the new cop.

This requires that the user have a VersionLocked - unless he or she does, all new cops would be effectively enabled as if the VersionLocked is set to the current version.

WDYT?

@Darhazer Darhazer modified the milestone: 1.0.0 Jun 27, 2019
@Darhazer Darhazer changed the title [Fix #5979] Introduce cops with special status [WIP] [Fix #5979] Introduce cops with special status Jun 27, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 27, 2019

The other issue is that the newly introduced cops would not be applied to the rubocop codebase as well unless we introduce an option to override the new status (e.g. enable all non-configured cops)

I've always assumed we'd just enable those cops in RuboCop's own config file. That's not really an issue I think, as the default config and RuboCop's config are orthogonal concerns.

Perhaps a better approach would be to have a VersionLocked in the user's .rubocop.yml and for cops introduced in later versions - to check if there is Enabled in the user's config, and if no - warn the user about the new cop.

This was discussed early on, but I decided it's not a good option mostly because of what you said - you need to keep updating this version. This PR is one of the few essential bits before releasing RuboCop 1.0 and after we cut we won't be enabling any new cops by default until 2.0, which means that the VersionLocked will come for free. ESLint made this approach pretty popular and I think it's reasonable, that's why I'm looking for us to emulate it.

We'll still have to refine the current behaviour and maybe add some options to suppress those messages, always enable new cops or always disable them. That's pretty simple, though.

@Darhazer
Copy link
Member Author

Darhazer commented Jun 28, 2019

I've always assumed we'd just enable those cops in RuboCop's own config file.

If that is the case, I'll remove the code that suppresses warnings during tests
Also, currently it warns during each config load (I just wanted to get the scope of the changes where there is a cop with a none status) while it should do it on the merged config. I'll update that as well

@@ -125,10 +125,14 @@ def enabled(config, only, only_safe = false)

def enabled?(cop, config, only_safe)
cfg = config.for_cop(cop)

# cfg['Enabled'] might be a string `none`, which is considered disabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pending or new might be a better name for this. Nil might be an option as well, although it's not very descriptive.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 1, 2019

Fine by me. We'll also need to add a few lines in the manual about this.

@rubocop-hq/rubocop-core That's a very important change, so I'd appreciate your input as well.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 9, 2019

Well, I'll assume that everyone's fine with my idea then. :D

@pirj
Copy link
Member

pirj commented Aug 23, 2019

The other issue is that the newly introduced cops would not be applied to the rubocop codebase as well unless we introduce an option to override the new status (e.g. enable all non-configured cops)

Do you mean when working on rubocop itself?
But there are config/default.yml where it's set to none/pending, and .rubocop.yml, where it can be configured. During internal investigations, the warnings will appear just like for any other repository.
Did I understand the problem correctly?

@pirj
Copy link
Member

pirj commented Aug 23, 2019

Does this mean that all the cops will be added to config/default.yml with none/pending/false statuses only from now on?

@pirj
Copy link
Member

pirj commented Aug 23, 2019

First, the need to disable the warning in 'cli spec behavior' context

I'm sorry, I'm out of context with this. What does that mean exactly?

@Darhazer
Copy link
Member Author

Do you mean when working on rubocop itself?

Yes. But the .rubocop.yml file solves the issue

Does this mean that all the cops will be added to config/default.yml with none/pending/false statuses only from now on?

Yes.

I'm sorry, I'm out of context with this. What does that mean exactly?

It doesn't matter if RuboCop uses its own config file that enabled those cops as @bbatsov suggested

@pirj
Copy link
Member

pirj commented Dec 4, 2019

I've moved this a bit forward:

  • moved the check to config_loader.rb, since otherwise if something was marked as pending in default/config.yml, the warning was emitted despite it was overridden in .rubocop.yml
  • renamed the status from new to pending

TODO:

  • update from master
  • when there's no project config file it should warn about pending cops (now it returns too early to warn)
  • when DisabledByDefault is on - pending cops should be disabled, when EnabledByDefault is on - pending cops should be enabled (didn't check, maybe works out of the box
  • write/fix specs

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 4, 2019

@pirj Thanks for the update!

@Darhazer
Copy link
Member Author

Closing in favor of #7567

@Darhazer Darhazer closed this Dec 13, 2019
@Darhazer Darhazer deleted the introduce-new-cops branch December 13, 2019 10:45
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

Successfully merging this pull request may close these issues.

None yet

3 participants