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

RuboCop does not merge Exclude by default and therefore scans vendor/bundle #9325

Open
eregon opened this issue Jan 2, 2021 · 6 comments
Open

Comments

@eregon
Copy link

eregon commented Jan 2, 2021

RuboCop currently does not merge Exclude by default.
I think this is a significant usability bug, because it makes RuboCop harder to use in combination with the very common Bundler path vendor/bundle.

Notably, it causes extra troubles for ruby/setup-ruby in ruby/setup-ruby#136 and RuboCop seems by far the most common tool to be tripped up by files under vendor/. Other tools seem to deal with it just fine by default, or be much less frequently used.

So that even though RuboCop does exclude vendor/**/* in the default config,
that has actually no effect at all (!) when using a .rubocop.yml such as:

AllCops:
  Exclude:
    - 'gemfiles/**/*'

Expected behavior

RuboCop should by default ignore vendor/**/*, even if the config defines extra AllCops: -> Exclude:.

Actual behavior

An extra AllCops: -> Exclude: overrides the default, which I believe is always unintentional.
This results in confusing errors, for instance when seeing an old TargetRubyVersion from a gem under vendor/bundle, such as:

$ bundle exec rubocop
Error: Unsupported Ruby version 2.1 found in `TargetRubyVersion` parameter (in vendor/bundle/ruby/2.5.0/gems/rainbow-3.0.0/.rubocop.yml). 2.1-compatible analysis was dropped after version 0.58.
Supported versions: 2.2, 2.3, 2.4, 2.5, 2.6

If it does not error, it would also slow down RuboCop significantly by scanning all these extra files.

This problem will also cause .git, node_modules and tmp to be scanned, while they should not.

Workaround

inherit_mode:
  merge:
    - Exclude

(https://docs.rubocop.org/rubocop/0.88/configuration.html#merging-arrays-using-inherit_mode)

I don't think it's a good workaround. RuboCop should work fine with the most used Bundler path without any extra manual step.
It is confusing that the default Exclude paths are completely forgotten whenever adding a custom Exclude.

Related: #288 (comment) #7819 and I guess other issues, this is counter-intuitive and confusing behavior.

@marcandre
Copy link
Contributor

marcandre commented Jan 3, 2021

Agreed.

Is there any use case where that could be problematic? (Asking @bbatsov, @koic and others that have more experience with config and uses)

@jonas054
Copy link
Collaborator

jonas054 commented Jan 5, 2021

The default list of excludes is small and only contains directories that are hard to imagine anyone would want to inspect.

So merging by default makes a lot of sense. The only potential problem I can foresee is that users who were overriding AllCops: Exclude and actually wanted to inspect files under tmp/ for example, would get those files excluded. Should be very rare.

@januszm
Copy link

januszm commented Jan 16, 2021

With more and more projects migrating to Github Actions, this will be a growing problem (In GH Actions gems install to vendor/bundle, that's how I found this PR)

cmur2 added a commit to zeebe-io/zeebe-client-ruby that referenced this issue Feb 3, 2021
cmur2 added a commit to zeebe-io/zeebe-client-ruby that referenced this issue Feb 3, 2021
cmur2 added a commit to zeebe-io/zeebe-client-ruby that referenced this issue Feb 3, 2021
@eregon
Copy link
Author

eregon commented Feb 20, 2021

Ping, it seems many people are hitting this when using GitHub Actions, it would be great if this was fixed in RuboCop.

jonas054 added a commit to jonas054/rubocop that referenced this issue Mar 20, 2021
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.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 22, 2021

See #9624.

@maxveldink
Copy link

I'm catching up on the conversation and late to the party here, but this is a great change I'd love to see merged. This issue has bit me three or four times in the last few months as I've been setting up CI for a few projects, and it's very frustrating when you hit the first instance where you need to Exclude a path, and suddenly CI begins failing for a seemingly unrelated issue.

I understand the implication of changing the default configuration behavior here; in the short term, perhaps figuring out how to warn about the behavior if the Exclude option is detected in the configuration would be a helpful addition. Also, add a way to suppress the warning as well?

Open to feedback here, but having something give me a clue that I'm overriding a pretty important default (as far as CI is concerned) would be nice!

tagliala added a commit to tagliala/activeadmin that referenced this issue Mar 4, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git` and `node_modules`.

Ref: rubocop/rubocop#9325

This commit also:
- Sorts list
- Excludes all `vendor` folders in `gemfiles` subfolder

### `master` (third run)

```
real	0m5.448s
user	0m5.614s
sys	0m1.096s
```

### This branch (third run)

```
real	0m2.431s
user	0m2.738s
sys	0m0.869s
```
tagliala added a commit to activeadmin/activeadmin that referenced this issue Mar 4, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git` and `node_modules`.

Ref: rubocop/rubocop#9325

This commit also:
- Sorts list
- Excludes all `vendor` folders in `gemfiles` subfolder

### `master` (third run)

```
real	0m5.448s
user	0m5.614s
sys	0m1.096s
```

### This branch (third run)

```
real	0m2.431s
user	0m2.738s
sys	0m0.869s
```
tagliala added a commit to diowa/ruby2-rails5-bootstrap-heroku that referenced this issue Mar 5, 2024
Also fix RuboCop config to exclude .git

Ref: rubocop/rubocop#9325
tagliala added a commit to diowa/ruby3-rails6-bootstrap-heroku that referenced this issue Mar 5, 2024
Also fix RuboCop config to exclude .git

Ref: rubocop/rubocop#9325
tagliala added a commit to diowa/ruby3-rails7-bootstrap-heroku that referenced this issue Mar 5, 2024
Also fix RuboCop config to exclude .git

Ref: rubocop/rubocop#9325
tagliala added a commit to tagliala/caxlsx that referenced this issue Apr 10, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git` and `node_modules`.

### Before:

```
real	0m3.699s
user	0m3.595s
sys	0m0.653s
```

### After:

```
real	0m1.712s
user	0m1.671s
sys	0m0.600s
```

Ref: rubocop/rubocop#9325
tagliala added a commit to tagliala/caxlsx that referenced this issue Apr 10, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git` and `node_modules`.

### Before:

```
real	0m3.699s
user	0m3.595s
sys	0m0.653s
```

### After:

```
real	0m1.712s
user	0m1.671s
sys	0m0.600s
```

Ref: rubocop/rubocop#9325
tagliala added a commit to ifad/chronomodel that referenced this issue Apr 20, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git`.

Ref: rubocop/rubocop#9325
tagliala added a commit to ifad/chronomodel that referenced this issue Apr 20, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git`.

Ref: rubocop/rubocop#9325
tagliala added a commit to ifad/hawk that referenced this issue Apr 20, 2024
The default `Exclude:` is not inherited when a custom value is provided,
so the performance was degraded because time was spent searching in
`.git`.

Ref: rubocop/rubocop#9325
JoeCohen added a commit to MushroomObserver/mushroom-observer that referenced this issue Aug 8, 2024
RuboCop was ignoring some of the todo file's Exclude's. This PR fixes most of that
- Applies the workaround suggested [here](rubocop/rubocop#13091 (comment)) and [here](rubocop/rubocop#9325 (comment))
- regenerates RuboCop todo file
- removes now-unnecessary `disable`'s in `app/helpers/forms_helper.rb`
- inlines the remaing `disable`; using the inline gives us one fewer place to change in case we want to remove this temporararily or permanently.
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

6 participants