Don't load configuration files outside of the current project unless there's no project specific configuration #8314
Conversation
Previously, the meaning of `FileFinder.root_level` would be "first non searched directory", now it is "last searched directory". This is more intuitive in my opinion (I assumed this behavior from the beginning until I found out it was not like that), but also more "correct", because it allows you to search up to the root directory if `FileFinder.root_level` is set to "/". Note that this `FileFinder.root_level?` logic is only useful for testing to provide an isolated environment not affected by the external file system where the tests are running. When rubocop runs "normally", it's ignored. However, in follow up work, I plan to start using these code paths, and this change allows me to stop artificially passing the parent of the directory where I want to stop searching.
deivid-rodriguez
force-pushed
the
full_fix_for_personal_configuration
branch
from
f58c72a
to
1d3b65f
Compare
If there's a specific project configuration, we don't want to pick up the exclusions in the user configuration. This doesn't happen already in the case of XDG setups, or if the HOME directory is not a parent of the project root directory. The specs currently configure such a setup, even if not very common in real life, maybe to workaround this issue. This PR fixes the problem by defining the root of project as the highest folder up in the directory hierarchy that contains a `Gemfile` or `gems.rb` file. Even if this definition matches most ruby projects, we could add additional fallbacks (like a `.gemspec` file?) to include more edge cases.
If by any chance, one happens to leave a `.rubocop.yml` file outside of my project, ignore it.
deivid-rodriguez
force-pushed
the
full_fix_for_personal_configuration
branch
from
1d3b65f
to
469ca61
Compare
| @@ -24,7 +24,7 @@ class << self | |||
|
|
|||
| attr_accessor :debug, :ignore_parent_exclusion, | |||
| :disable_pending_cops, :enable_pending_cops | |||
| attr_writer :default_configuration | |||
| attr_writer :default_configuration, :project_root | |||
There was a problem hiding this comment.
This is only for test code to be able to reset state, the project_root doesn't really change at runtime. I can alternatively revert this change and use instance_variable_set inside the test code to reset it.
| def project_root | ||
| @project_root ||= find_project_root | ||
| end | ||
|
|
There was a problem hiding this comment.
I intentionally left this method above in the public section and documented it since I believe it could be useful for extensions. At least it's useful for a cop I'm helping with in the rubocop-packaging extension.
|
Nicely done!
Probably Might be a good idea to add a few lines in the docs about not adding exclusions to the global config file, or potentially adding some validation and warnings when this is done. |
|
Thanks @bbatsov! 💜
I can prepare a followup PR to add this 👍.
I think there might be some situations where exclusions in the global configuration file make sense. Like if you have a lot of projects with a |
In previous PRs, #8176 and #8239, I fixed particular cases of this problem, but it was still not fixed in general. So, if I have my personal configuration at
~/.config/rubocop/config.ymland my project is at~/Code/my_project, then my personal configuration won't be loaded thanks to those PRs. However, if I have my personal configuration at~/.rubocop.yml, thenrubocopwill still load exclusions from there because$HOMEis an ancestor of the current project folder, and that's howfind_last_file_upwardscurrently works.To fix this problem in general, I made sure
find_last_file_upwards(and any configuration search in general) doesn't go past the current project's root. To do that, I needed to define what the root of a project is. While there were other options, like presence of a.gitfolder, for example, I decided go with "the highest folder up in the directory hierarchy containing a Gemfile or gems.rb" file.This is the same definition
bundleruses and I believe it makes sense, specially because the original problems I want to solve here manifest were personal configuration files try to load rubocop plugins outside of the current bundle, and thus making rubocop crash. See #7433. By using Gemfile presence as a criteria, we prevent exactly this kind of problem.As a note, the reason the test suite never catched this issue is that it intentionally defines a not very common folder structure where the current project folder is not inside
$HOME. I think that maybe the structure was defined this way in order to workaround this problem.Finally, although this should work for most ruby projects out there, projects without a
Gemfiledon't fit into this criteria. I believe we can leave it as it is tough, and wait for issues from such projects before adding more fallbacks to this "root of a project" definition.I'd be very happy to hear your thoughts, thanks for reading!
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.