rubocop
rubocop copied to clipboard
[Fix #9325] Make Exclude config parameters merge by default
First of all, I'm not convinced that this is a good idea. Let's discuss.
It turned out to be trickier to implement than I had anticipated. There was code here and there that depended on the inherit mode being override
for Exclude
. The changes in pretty central parts of the logic makes me worry that we're opening up a can of worms. More issues will be reported down the line. That sort of thing.
And also, the complexities of configuration inheritance are getting worse with this change IMO. It's a bit weird that Exclude
s are merged while Include
s are overridden. I experimented with letting Include
s be merged as well, but that's a much bigger change since default configuration uses Include
to let specific cops only inspect one kind of file.
So that's the case against. :slightly_smiling_face: Here's the description:
Add the inherit_mode
setting in default configuration to make Exclude
parameters in user configuration merge with the same Exclude
settings in inherited configuration. The result of rubocop --auto-gen-config
is affected by this change, since we otherwise copy values from inherited Exclude
settings into the generated file.
By the way, this is a change to default configuration, which means it requires bumping the major version of RuboCop. Right?
Yes. That's why we have to weigh the pros and cons of this very carefully.
Looks good to me.
I think almost nobody ever wants to scan files under:
Exclude:
- 'node_modules/**/*'
- 'tmp/**/*'
- 'vendor/**/*'
- '.git/**/*'
And it can still be done with
inherit_mode:
override:
- Exclude
if desired (as documented).
So I'm not sure as a RuboCop user that requiring a new major version for this is really needed. OTOH, if releasing RuboCop 2.x would be something reasonably soon then I wouldn't really mind.
Another way to do this which would avoid changing the default inherit_mode
for Exclude
would be to have something like DefaultExclude:
in addition to Exclude:
and then merge both of these arrays.
Then it would solve the issue, and for people wanting no default excludes, it'd be quite clear:
AllCops:
DefaultExclude: []
So I'm not sure as a RuboCop user that requiring a new major version for this is really needed.
I'm afraid that anything that will break RuboCop's behaviour for some users requires a major release. There are no plans for RuboCop 2.0 at this point, but it's bound to happen at some point.
Am I using the milestone feature correctly? I set 2.0 as milestone for this one, but does that mean that the PR should be merged exactly when we release 2.0 -- not before?
@jonas054 Yeah, you're using it correctly.
It would allow removing those tweaks from extensions: rubocop-rspec
, rubocop-rails
.
I experimented with letting Includes be merged as well, but that's a much bigger change since default configuration uses Include to let specific cops only inspect one kind of file.
It may seem that Include
is semantically different. Where Exclude
means "don't check those", and it makes sense to accumulate definitions from different places (default RuboCop config, gems, project root config, project sub-directory config). Include
feels different, and is supposed to mean "only check those", e.g.
Include:
- '**/Gemfile'
After checking the Include
statements from config/default.yml
of rubocop
, rubocop-rspec
and rubocop-rails
, I couldn't come up with an example where users would suffer from Include
merging by default. It was a revelation that it might make sense to add those as well, e.g.:
Rails/AddColumnIndex:
Include:
- db/migrate/*.rb
E.g. on my current project we have multiple databases, and migrations for the second one are in db/misc-migrate
.
Having Include
in override mode would skip the cop for db/migrate
.
much bigger change
Code-wise, or impact-wise bigger?
Code-wise, or impact-wise bigger?
I was thinking impact when I wrote that. My intuition says that changing the meaning from include only to include also would be big, but I haven't conducted any experiments to prove it.