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

Allow disabling of all cops from default Rubocop config #1204

Closed

Conversation

rickmzp
Copy link

@rickmzp rickmzp commented Jul 12, 2014

Sometimes it is preferred to configure the use of Rubocop with a whitelist instead of a blacklist. This option allows Rubocop users to build a style guide from scratch or from a small subset of existing style guides without needing to disable rules that are enabled by default.

@yujinakayama
Copy link
Collaborator

I understand the use case, but currently RuboCop's configuration system is being redesigned and the new system will allow you to do so (see Style.enabled = false snippet in #1151).

@rickmzp
Copy link
Author

rickmzp commented Jul 12, 2014

I appreciate that the configuration system will go through some heavy rethinking, but I don't think that should be a sole reason for preventing users from gaining these benefits today, especially as getting that new system implemented could take some time.

What's the harm in merging this in sooner than later and then marking this as deprecated when your solution is launched?

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 12, 2014

I'm hoping that @yujinakayama will complete the new config system for the upcoming RuboCop release, which would render any changes to the current config system irrelevant as they'll never see the light of day. Ultimately that's Yuji's call.

@rickmzp
Copy link
Author

rickmzp commented Jul 12, 2014

If it makes it into the next release, that would be fantastic. If it misses the deadline for when the next minor version of the gem is cut, then I think it would be a shame if this PR is still pending by that time.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 17, 2014

@jonas054 What do you think?

@jonas054
Copy link
Collaborator

Frankly, I don't understand the need. @rickmzp What's the problem with the currently supported way of working where your .rubocop.yml would look like this, if it was built from some existing style guides:

inherit_from:
  - path/to/github_rules.yml
  - path/to/sandi_metz_rules.yml
  - .rubocop_todo.yml

The difference from the whitelist concept is that you have to re-run with --auto-gen-config when cops are added or changed in new RuboCop versions, and there are new offenses. This, IMO, is a great feature. It alerts users to new possibilities with the tool.

With the --disable-default-cops flag you need to have an Enabled: true for each cop in the configuration files. That means we divide configuration files into blacklist and whitelist style configuration files. I don't think that's good.

@rickmzp
Copy link
Author

rickmzp commented Jul 17, 2014

Can you explain "I don't think that's good?"

@jonas054
Copy link
Collaborator

It's almost like two different file types. You have the normal configuration files that everybody have used up until now. They don't have Enabled: true in them so they can't be used with the --disable-default-cops flag. Then you have the configuration files intended to be used as whitelists. They can be used with or without the --disable-default-cops flag, but their Enabled: true settings are superfluous in the blacklist context.

Let's say that somebody provides a configuration file supporting the github style guide as a gem. You ask them "Can you please add Enabled: true parameters for each cop? I want to use this as a whitelist." I'm saying that's unnecessary.

@rickmzp
Copy link
Author

rickmzp commented Jul 17, 2014

That's right, this flag would not work in the scenario you described. Actually, the user of this flag would not be at all interested in using an external style config, because they are more interested in keeping a set of explicit style rules in a single place, instead of spread across different places, especially an external dependency. So, in reality, the superfluous flag example you described would not happen.

Convention is what you described. Configuration is what I'm providing.

@jonas054
Copy link
Collaborator

You can achieve the same thing by copying config/enabled.yml from the RuboCop code to your project root, replacing all Enabled: true in it with Enabled: false, and adding an inherit_from in .rubopcop.yml. The rest of the .rubopcop.yml file would act as a whitelist, and you wouldn't need to add a command line flag.

@rickmzp
Copy link
Author

rickmzp commented Jul 18, 2014

Indeed. At first, we considered creating a file called .rubocop_disable_defaults.yml doing what you described, but it felt a bit messy to commit that to our code. Then I thought that there had to be a better way, so I went ahead and created this PR.

Also, there might be a better place to put the flag other than the CLI, but it didn't seem as easy to implement with the current config architecture. So I would love better ideas in this regard without having to wait to refactor the whole thing.

@jonas054
Copy link
Collaborator

If we could find a way to add a parameter in a configuration file saying "this is a whitelist", I'd be happy. It would be much better, because it would not create that binding between how the configuration file is made up and what flags you need to use. Also, a parameter like that could be handled in a conversion tool for @yujinakayama's redesigned configuration system.

But as you say, @rickmzp, it might not be as easy to implement.

@rickmzp
Copy link
Author

rickmzp commented Jul 19, 2014

I completely agree. That coupling is pretty bad and I'm up for getting rid of it. I'll try to give it a shot this weekend and push it to this PR. I also like how that parameter would be compatible with a conversion tool for @yujinakayama's upcoming configuration system. The engineering required to get that to work would be worth it as it's better for the project than this current solution. Thanks!

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 19, 2014

Sounds good to me too.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 8, 2014

@rickmzp Any progress with this?

@bbatsov bbatsov closed this Sep 30, 2014
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

4 participants