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

[Fix #7433] User vs project exclusion inheritance #8176

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Jun 20, 2020

If there's a specific project configuration, we want the exclusions for the user configuration to be ignored, just like the rest of the configuration is ignored.

Note that this solution slightly changes behavior if people are running rubocop for within a subproject folder that resolves to a .rubocop.yml different from the one in the root folder. However, I believe that if people are doing this, they are probably using --ignore-parent-exclusion already, so it's not going to be an issue.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@deivid-rodriguez
Copy link
Contributor Author

There's an error which sounds like it's my fault. I'll have a look.

@deivid-rodriguez
Copy link
Contributor Author

Ok, so I tried this on a real project and it did "break" my editor integration when inspecting vendored code.

  • Without this PR, if I change a vendored file in a way that's not compliant with its own configuration, it will complain.
  • With this PR, my editor will simply ignore vendored code (excluded from root configuration).

I believe it's debatable which behaviour is best, but I'll try to change this PR to be fully backwards compatible with the previous behaviour in this regard and add a regression spec.

@deivid-rodriguez
Copy link
Contributor Author

Ok, I don't think I was testing this correctly. Turns out this PR is also correcting a very strange behaviour regarding vendored files.

The standard behavior of my editor rubocop integration is to ignore vendored files. That's unchanged with this PR.

However, if I put a ~/.rubocop.yml file in my home folder (with any content, as far as I can see), then my editor integration starts checking the vendored file for some reason 🤨. This PR also fixes that issue.

For what it's worth, my editor integration is https://github.com/dense-analysis/ale/, and the rubocop command run under the hood looks like: bin/rubocop --format json --force-exclusion --stdin /home/deivid/Code/activeadmin/vendor/bundle/ruby/2.7.0/bundler/gems/rubocop-1fe8dd1d8582/lib/rubocop.rb < /tmp/nvimchZ4vE/1/rubocop.rb.

lib/rubocop/file_finder.rb Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez force-pushed the always_ignore_personal_if_project_present branch 2 times, most recently from f19adaa to 3e4a7ef Compare June 23, 2020 18:42
Copy link
Collaborator

@jonas054 jonas054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog entry now needs to be moved up to the master (unreleased) section, but other than that the changes look good. 👍

Don't load personal configuration files to look for exclusions if
there's a project configuration.
@deivid-rodriguez deivid-rodriguez force-pushed the always_ignore_personal_if_project_present branch from 3e4a7ef to 2362fe7 Compare July 4, 2020 08:11
@deivid-rodriguez
Copy link
Contributor Author

@jonas054 Thanks!! I rebased this PR and moved the changelog entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants