[Fix #9325] Make Exclude config parameters merge by default

[jonas054]

trafficstars

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 Excludes are merged while Includes are overridden. 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.

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.

[jonas054]

By the way, this is a change to default configuration, which means it requires bumping the major version of RuboCop. Right?

[bbatsov]

Yes. That's why we have to weigh the pros and cons of this very carefully.

[eregon]

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: []

[bbatsov]

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.

[jonas054]

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?

[bbatsov]

@jonas054 Yeah, you're using it correctly.

[pirj]

It would allow removing those tweaks from extensions: rubocop-rspec, rubocop-rails.

[pirj]

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?

[jonas054]

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.