[Fix #9752] Improve error message for top level department #9936
Conversation
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.
jonas054
force-pushed
the
9752_department_hierarchy
branch
from
bf7edd2
to
e4147c8
Compare
|
I love this change! Simple, but effective! |
|
This is great, thanks! However, I really want to be able to disable all chef departments sometimes without having to keep up with the list of all of them, which change over time. So I can think of two approaches:
I think the former is cleaner and provides an even better base for people to continue building powerful tools on the amazing Rubocop base, but the latter would do as well. |
|
I considered alternative 1, but decided on the approach in this PR, because I wasn't sure what the consequences could be if we introduce actual department hierarchies. There is a variant of alternative 2 that you can use today without any changes to RuboCop. It uses ERB pre-processing. # .rubocop.yml
<% RuboCop::Cop::Registry.global.departments.grep(%r{\AChef/}).each do |dept| %>
<%= dept %>:
Enabled: true
<% end %>I'm not 100% sure that the APIs used on the first line are considered public APIs. I guess @bbatsov or @koic would know. |
|
Ohhhhhhh that's neat. I didn't know the config file was passed through ERB! I can do all sorts of neat things with that (that I shouldn't). That'll be a really useful short-term solution. What do you think about # 1, but doing it at a major version in case it's a breaking change? |
|
You also have to keep in mind the added complexity of such a change. If there's not a lot of demand for such a change, I'd rather avoid it. The codebase related to configuration is already very complex and we're quite wary of making it even more complex. |
This is a smaller change than the one suggested in #9752. Instead of allowing to use top level departments in configuration, I just made a better error message. I think this change has a lower risk of causing new problems down the line.
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 aChefdepartment that can disable all departments under it. This is not the case, but the error messageError: unrecognized cop Chef found in .cookstyle.ymlis 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. Example:
Also, we change the wording of
unrecognized coptounrecognized cop or department, and extend the did-you-mean functionality to also look for similar department names.