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

Don't load exclusions from personal files when passing a config with a custom name #8239

Merged

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Jul 5, 2020

If passing a configuration with a custom name, like rubocop --config .my_custom_project_config.yml, that configuration should act as the project configuration and we want personal configuration files disregarded.

That's actually the only case where find_files_upwards can return an empty list, because when given an existing .rubocop.yml as the starting point, it will at least return that file, and the fallback to personal config files will not be applied.

Well, there's another case where find_files_upwards could return an empty list, which is when giving it a non existent file as the starting point. However, I don't think that ever happens in real life, and in tests it was only happening once, and due to a typo I believe.

So, we can completely remove the explicit fallback to look for exclusions in personal configuration files.

  • 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

This is a follow up to #8176. @jonas054 what do you think?

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 6, 2020

If passing a configuration with a custom name, like rubocop --config-file .my_custom_project_config.yml, that configuration should act as the project configuration and we want personal configuration files disregarded.

What do you call personal configuration? Some examples might help understand better the problem you're trying so solve.

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jul 6, 2020

Hi @bbatsov, sorry for not being clear.

When I talk about personal configuration files, I mean configuration files in either $HOME or $XDG_CONFIG folders.

Currently these files are not very useful IMO because they get loaded in situations where you don't want them loaded. See for example #7433. In my opinion, they should get only loaded as a fallback when the project has no specific rubocop configuration, or no explicit configuration is being passed to the CLI.

I'm submitting a series of PRs to fix these issues.

  • In [Fix #7433] User vs project exclusion inheritance #8176 I fix the issue when there's an explicit project configuration and neither $HOME, or $XDG_CONFIG are parent folders of the current project.
  • In this PR I fix the issue when there's an explicit configuration given on the command line.
  • In a followup PR, I'd like to fix the problem in general but I'm trying small changes first that can be easily justified.

A practical example of this specific fix would be when trying to add some initial rubocop configuration to a project. You might want to first try different "template" configurations with different settings, and the --config option can help there. In that case, I don't think you want your personal ~/.rubocop.yml to influence the configuration you're testing.

@deivid-rodriguez
Copy link
Contributor Author

By the way, if we can find a better terminology, I'm happy to change this and the already merged changelog entries. Maybe "user configuration" is better since it matches more closely the terminology used by the code. I've also seen "global configuration" used.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 6, 2020

Currently these files are not very useful IMO because they get loaded in situations where you don't want them loaded. See for example #7433. In my opinion, they should get only loaded as a fallback when the project has no specific rubocop configuration, or no explicit configuration is being passed to the CLI.

Agreed. I had almost forgotten about them at this point.

@deivid-rodriguez
Copy link
Contributor Author

I had almost forgotten about them at this point.

Me too! I tried using them at some point but eventually gave up, because they kept getting in the middle. Then I saw #7433 and realized other people were trying to use them too, so I decided to give it another try.

@deivid-rodriguez
Copy link
Contributor Author

I rebased this PR and moved the changelog entry in response to the new release 😃

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 7, 2020

I think you should also update the docs to reflect this.

…custom name

If passing a configuration with a custom name, like `rubocop
--config-file .my_custom_project_config.yml`, that configuration should
act as the project configuration and we want personal configuration
files disregarded.

That's actually the only case where `find_files_upwards` can return an
empty list, because when given an existing `.rubocop.yml` as the
starting point, it will at least return that file, and the fallback to
personal config files will not be applied.

Well, there's another case where `find_files_upwards` could return an
empty list, which is when giving it a non existent file as the starting
point. However, I don't think that ever happens in real life, and in
tests it was only happening once, and due to a typo I believe.

So, we can completely remove the explicit fallback to look for
exclusions in personal configuration files.
@deivid-rodriguez
Copy link
Contributor Author

@bbatsov I added a note about the --config flag which I believe was undocumented. (Also changed my previous comments in this PR since they were mentioning --config-file instead, which doesn't exist 😅).

Regarding the behaviour of looking up the highest config file to look for exclusions, there's this: https://github.com/rubocop-hq/rubocop/blob/master/docs/modules/ROOT/pages/configuration.adoc#include-and-exclude-are-relative-to-their-directory.

In my opinion, the problem with that docs is that they don't explain why this is done differently from everything else. I'm currently investigating the why, since I think there's no good reason for this and all these code could eventually go. But until then I'm not sure I can improve those docs. From the current wording I don't think it's implied that user configuration files should be searched, so I'd say this is just a bug fix that doesn't need documentation.

@bbatsov bbatsov merged commit ee4f5dc into rubocop:master Jul 7, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 7, 2020

Fair enough! Thanks for tackling this!

@deivid-rodriguez
Copy link
Contributor Author

No problem! I'll follow up with more stuff!

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