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

department-level enable/disable does not work #9752

Closed
jaymzh opened this issue Apr 29, 2021 · 6 comments
Closed

department-level enable/disable does not work #9752

jaymzh opened this issue Apr 29, 2021 · 6 comments
Assignees

Comments

@jaymzh
Copy link

jaymzh commented Apr 29, 2021

UPDATE: On further investigation, department-level enable/disable just doesn't work.

The docs say this should work works:

All cops are then disabled by default. Only cops appearing in user configuration files are enabled. Enabled: true does not have to be set for cops in user configuration. They will be enabled anyway. It is also possible to enable entire departments by adding for example

Style:
  Enabled: true

But it turns out this doesn't work at all. This is because Style is seen as an "invalid" config in Config::Validator.validate() and thus ignored. :/


Original ticket (for context)

This is probably not a rubocop bug, but I dunno where else to ask.

In the cookstyle project, we just sit ontop of the awesomeness that is Rubocop, and define a bunch of our own Cops and departments.

However, departments defined by our own cops don't seem to be enable-able the way, say Style is. So I can do this:

AllCops:
  DisabledByDefault: true

Style:
  Enabled: true

But what I can't do is (Chef/Correctness here is a department, you can see code for cops in that department here).

AllCops:
  DisabledByDefault: true

Chef/Correctness:
  Enabled: true

or even:

AllCops:
  DisabledByDefault: true

Chef:
  Enabled: true

What's really strange is that in robocop I don't see any special setup for the Style or Layout etc modules, so I would think this would "just work"... but it doesn't seem to. When I we try we get:

Warning: Chef/Correctness does not support Enabled parameter.

Supported parameters are: 

  - StyleGuideBaseURL

If I do just Chef, then I get:

Error: unrecognized cop Chef found in .cookstyle.yml

I'm sure is some minor dumb thing we're doing or not doing that's preventing this from working like the rubocop-built-in departments.

Any help would be greatly appreciated.

@jaymzh jaymzh changed the title how do department enables work? department-level enable/disable does not work Apr 30, 2021
@jonas054
Copy link
Collaborator

jonas054 commented Jul 4, 2021

I don't understand. Invalid configuration should make RuboCop stop and report the error, not be ignored. Do you get any error or warning message?

@jonas054
Copy link
Collaborator

I wrote the above because you said that department level enable/disable doesn't work at all. And that sounds strange to me, because I know that it basically works. 🙂

About the original ticket, I can see that Chef/Correctness does not support Enabled parameter is reported because there needs to be an Enabled: true in the default configuration of the department. Otherwise it can't be overridden.

Also, there's no concept of department hierarchy implemented in RuboCop, so Chef doesn't exist as a department and it cannot be disabled.

I hope that helps.

@jaymzh
Copy link
Author

jaymzh commented Jul 11, 2021

OK, so department is always everything up until the last slash, as opposed to everything up to the first slash.

So if you have a rule A/B/C/D you can disable A/B/C (if it has a Enabled default) or A/B/C/D, but never A/B or A, is that correct?

I can throw a PR at the chefstyle folks to add a default to Chef/*... how hard would it be to extend rubocop to understand department hierarchies?

Thanks!

@jonas054
Copy link
Collaborator

OK, so department is always everything up until the last slash, as opposed to everything up to the first slash.

Yes.

So if you have a rule A/B/C/D you can disable A/B/C (if it has a Enabled default) or A/B/C/D, but never A/B or A, is that correct?

Yes.

I can throw a PR at the chefstyle folks to add a default to Chef/*...

Great!

how hard would it be to extend rubocop to understand department hierarchies?

It's one of those things that feel like it should be fairly easy, but you can't say for sure until you've actually done it. I can try to implement it. Let's keep this issue open for a while.

@jonas054 jonas054 self-assigned this Jul 12, 2021
jonas054 added a commit to jonas054/rubocop that referenced this issue Jul 16, 2021
When there are cop departments that seem to have a structure, like in
the cookstyle gem where departments are named `Chef/Style`,
`Chef/Correctness`, etc., it's easy to assume that there is a `Chef`
department that can disable all departments under it. This is not the
case, but the error message
`Error: unrecognized cop Chef found in .cookstyle.yml` is not
informative enough.

If an unrecognized cop or department is the first part of existing
departments, we list the existing departments. This should help the user
realize what needs to be changed.

Also, we change the wording of `unrecognized cop` to
`unrecognized cop or department`, and extend the did-you-mean
functionality to also look for similar department names.
thearjunmdas pushed a commit to thearjunmdas/rubocop that referenced this issue Aug 20, 2021
When there are cop departments that seem to have a structure, like in
the cookstyle gem where departments are named `Chef/Style`,
`Chef/Correctness`, etc., it's easy to assume that there is a `Chef`
department that can disable all departments under it. This is not the
case, but the error message
`Error: unrecognized cop Chef found in .cookstyle.yml` is not
informative enough.

If an unrecognized cop or department is the first part of existing
departments, we list the existing departments. This should help the user
realize what needs to be changed.

Also, we change the wording of `unrecognized cop` to
`unrecognized cop or department`, and extend the did-you-mean
functionality to also look for similar department names.
@jaymzh
Copy link
Author

jaymzh commented Jan 19, 2022

Hey @jonas054 since you closed this, should I open up a separate enhancement for hierarchical departments?

@jonas054
Copy link
Collaborator

Yes, feel free to do so. I'll take a look at the problem. Please tag me in the new issue and reference this issue in it.

jaymzh added a commit to jaymzh/cookstyle that referenced this issue Jan 22, 2022
Over in rubocop/rubocop#9752 we figured out
that the reason end-users can't enable/disable Cookstyle departments is
because `Enabled` isn't in the config and thus [can't be
overridden](rubocop/rubocop#9752 (comment)).

This change will allow a user to not only do the obvious:

```yaml
Chef/Correctness:
  Enabled: false
```

But more interestingly do something like this:

```yaml
AllCops:
  DisabledByDefault: true

Chef/Correctness:
  Enabled: true
```

CC @jonas054
jaymzh added a commit to jaymzh/cookstyle that referenced this issue Jan 22, 2022
Over in rubocop/rubocop#9752 we figured out
that the reason end-users can't enable/disable Cookstyle departments is
because `Enabled` isn't in the config and thus [can't be
overridden](rubocop/rubocop#9752 (comment)).

This change will allow a user to not only do the obvious:

```yaml
Chef/Correctness:
  Enabled: false
```

But more interestingly do something like this:

```yaml
AllCops:
  DisabledByDefault: true

Chef/Correctness:
  Enabled: true
```

CC @jonas054
jaymzh added a commit to jaymzh/cookstyle that referenced this issue Jan 22, 2022
Over in rubocop/rubocop#9752 we figured out
that the reason end-users can't enable/disable Cookstyle departments is
because `Enabled` isn't in the config and thus [can't be
overridden](rubocop/rubocop#9752 (comment)).

This change will allow a user to not only do the obvious:

```yaml
Chef/Correctness:
  Enabled: false
```

But more interestingly do something like this:

```yaml
AllCops:
  DisabledByDefault: true

Chef/Correctness:
  Enabled: true
```

CC @jonas054

Signed-off-by: Phil Dibowitz <phil@ipom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants