From 1aab1a62bfb8d252b637937bc051cff19c3019b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Sun, 12 Jul 2020 15:40:58 +0200 Subject: [PATCH 1/4] Fix `traverse_files_upwards` to make searching up to the root possible 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. --- lib/rubocop/file_finder.rb | 4 ++-- lib/rubocop/rspec/shared_contexts.rb | 15 ++++++++------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/rubocop/file_finder.rb b/lib/rubocop/file_finder.rb index 05dbb3ccd0a..b6724f79163 100644 --- a/lib/rubocop/file_finder.rb +++ b/lib/rubocop/file_finder.rb @@ -32,10 +32,10 @@ def find_last_file_upwards(filename, start_dir) def traverse_files_upwards(filename, start_dir) Pathname.new(start_dir).expand_path.ascend do |dir| - break if FileFinder.root_level?(dir) - file = dir + filename yield(file.to_s) if file.exist? + + break if FileFinder.root_level?(dir) end end end diff --git a/lib/rubocop/rspec/shared_contexts.rb b/lib/rubocop/rspec/shared_contexts.rb index d3fb2081d60..7bc0b59349f 100644 --- a/lib/rubocop/rspec/shared_contexts.rb +++ b/lib/rubocop/rspec/shared_contexts.rb @@ -12,16 +12,17 @@ # get mismatched pathnames when loading config files later on. tmpdir = File.realpath(tmpdir) + virtual_home = File.expand_path(File.join(tmpdir, 'home')) + Dir.mkdir(virtual_home) + ENV['HOME'] = virtual_home + ENV.delete('XDG_CONFIG_HOME') + + working_dir = File.join(tmpdir, 'work') + # Make upwards search for .rubocop.yml files stop at this directory. - RuboCop::FileFinder.root_level = tmpdir + RuboCop::FileFinder.root_level = working_dir begin - virtual_home = File.expand_path(File.join(tmpdir, 'home')) - Dir.mkdir(virtual_home) - ENV['HOME'] = virtual_home - ENV.delete('XDG_CONFIG_HOME') - - working_dir = File.join(tmpdir, 'work') Dir.mkdir(working_dir) Dir.chdir(working_dir) do From f0adb9f970cc01229e7ce9e55c0ca4ddc1a5421a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Sat, 20 Jun 2020 19:47:43 +0200 Subject: [PATCH 2/4] Only look up to the project's root for exclusions 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. --- lib/rubocop/config_loader.rb | 16 ++++++++++++++- lib/rubocop/file_finder.rb | 16 +++++++-------- lib/rubocop/rspec/shared_contexts.rb | 3 ++- spec/rubocop/config_loader_spec.rb | 29 ++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 10 deletions(-) diff --git a/lib/rubocop/config_loader.rb b/lib/rubocop/config_loader.rb index e03b040e4d6..60064c510ca 100644 --- a/lib/rubocop/config_loader.rb +++ b/lib/rubocop/config_loader.rb @@ -118,7 +118,7 @@ def possible_new_cops?(config) end def add_excludes_from_files(config, config_file) - exclusion_file = find_last_file_upwards(DOTFILE, config_file) + exclusion_file = find_last_file_upwards(DOTFILE, config_file, project_root) return unless exclusion_file return if PathUtil.relative_path(exclusion_file) == PathUtil.relative_path(config_file) @@ -134,6 +134,12 @@ def default_configuration end end + # Returns the path rubocop inferred as the root of the project. No file + # searches will go past this directory. + def project_root + @project_root ||= find_project_root + end + def warn_on_pending_cops(pending_cops) return if pending_cops.empty? @@ -161,6 +167,14 @@ def find_project_dotfile(target_dir) find_file_upwards(DOTFILE, target_dir) end + def find_project_root + pwd = Dir.pwd + gems_file = find_last_file_upwards('Gemfile', pwd) || find_last_file_upwards('gems.rb', pwd) + return unless gems_file + + File.dirname(gems_file) + end + def find_user_dotfile return unless ENV.key?('HOME') diff --git a/lib/rubocop/file_finder.rb b/lib/rubocop/file_finder.rb index b6724f79163..1d4522f41ec 100644 --- a/lib/rubocop/file_finder.rb +++ b/lib/rubocop/file_finder.rb @@ -9,20 +9,20 @@ def self.root_level=(level) @root_level = level end - def self.root_level?(path) - @root_level == path.to_s + def self.root_level?(path, stop_dir) + (@root_level || stop_dir) == path.to_s end - def find_file_upwards(filename, start_dir) - traverse_files_upwards(filename, start_dir) do |file| + def find_file_upwards(filename, start_dir, stop_dir = nil) + traverse_files_upwards(filename, start_dir, stop_dir) do |file| # minimize iteration for performance return file if file end end - def find_last_file_upwards(filename, start_dir) + def find_last_file_upwards(filename, start_dir, stop_dir = nil) last_file = nil - traverse_files_upwards(filename, start_dir) do |file| + traverse_files_upwards(filename, start_dir, stop_dir) do |file| last_file = file end last_file @@ -30,12 +30,12 @@ def find_last_file_upwards(filename, start_dir) private - def traverse_files_upwards(filename, start_dir) + def traverse_files_upwards(filename, start_dir, stop_dir) Pathname.new(start_dir).expand_path.ascend do |dir| file = dir + filename yield(file.to_s) if file.exist? - break if FileFinder.root_level?(dir) + break if FileFinder.root_level?(dir, stop_dir) end end end diff --git a/lib/rubocop/rspec/shared_contexts.rb b/lib/rubocop/rspec/shared_contexts.rb index 7bc0b59349f..9469089a85c 100644 --- a/lib/rubocop/rspec/shared_contexts.rb +++ b/lib/rubocop/rspec/shared_contexts.rb @@ -17,7 +17,8 @@ ENV['HOME'] = virtual_home ENV.delete('XDG_CONFIG_HOME') - working_dir = File.join(tmpdir, 'work') + base_dir = example.metadata[:project_inside_home] ? virtual_home : tmpdir + working_dir = File.join(base_dir, 'work') # Make upwards search for .rubocop.yml files stop at this directory. RuboCop::FileFinder.root_level = working_dir diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index d08b2ea6c26..a487b154188 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/spec/rubocop/config_loader_spec.rb @@ -197,6 +197,35 @@ end end + context 'when project has a Gemfile', :project_inside_home do + let(:file_path) { '.rubocop.yml' } + + before do + create_empty_file('Gemfile') + + create_file(file_path, <<~YAML) + AllCops: + Exclude: + - vendor/** + YAML + end + + context 'and there is a personal config file in the home folder' do + before do + create_file('~/.rubocop.yml', <<~YAML) + AllCops: + Exclude: + - tmp/** + YAML + end + + it 'ignores personal AllCops/Exclude' do + excludes = configuration_from_file['AllCops']['Exclude'] + expect(excludes).to eq([File.expand_path('vendor/**')]) + end + end + end + context 'when a parent file specifies DisabledByDefault: true' do let(:file_path) { '.rubocop.yml' } From 12d80e56312176015c3986c3e210961fc5f376f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Sun, 12 Jul 2020 20:03:18 +0200 Subject: [PATCH 3/4] Ignore configuration files outside of the project If by any chance, one happens to leave a `.rubocop.yml` file outside of my project, ignore it. --- lib/rubocop/config_loader.rb | 4 ++-- lib/rubocop/rspec/shared_contexts.rb | 5 +++-- spec/rubocop/config_loader_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/rubocop/config_loader.rb b/lib/rubocop/config_loader.rb index 60064c510ca..b04bd49f962 100644 --- a/lib/rubocop/config_loader.rb +++ b/lib/rubocop/config_loader.rb @@ -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 alias debug? debug alias ignore_parent_exclusion? ignore_parent_exclusion @@ -164,7 +164,7 @@ def merge_with_default(config, config_file, unset_nil: true) private def find_project_dotfile(target_dir) - find_file_upwards(DOTFILE, target_dir) + find_file_upwards(DOTFILE, target_dir, project_root) end def find_project_root diff --git a/lib/rubocop/rspec/shared_contexts.rb b/lib/rubocop/rspec/shared_contexts.rb index 9469089a85c..b2bafec34c8 100644 --- a/lib/rubocop/rspec/shared_contexts.rb +++ b/lib/rubocop/rspec/shared_contexts.rb @@ -18,13 +18,14 @@ ENV.delete('XDG_CONFIG_HOME') base_dir = example.metadata[:project_inside_home] ? virtual_home : tmpdir - working_dir = File.join(base_dir, 'work') + root = example.metadata[:root] + working_dir = root ? File.join(base_dir, 'work', root) : File.join(base_dir, 'work') # Make upwards search for .rubocop.yml files stop at this directory. RuboCop::FileFinder.root_level = working_dir begin - Dir.mkdir(working_dir) + FileUtils.mkdir_p(working_dir) Dir.chdir(working_dir) do example.run diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index a487b154188..d93b95a5b2b 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/spec/rubocop/config_loader_spec.rb @@ -82,6 +82,26 @@ end end + context 'when there is a spurious rubocop config outside of the project', root: 'dir' do + let(:dir_path) { 'dir' } + + before do + # Force reload of project root + described_class.project_root = nil + create_empty_file('Gemfile') + create_empty_file('../.rubocop.yml') + end + + after do + # Don't leak project root change + described_class.project_root = nil + end + + it 'ignores the spurious config and falls back to the provided default file if run from the project' do + expect(configuration_file_for).to end_with('config/default.yml') + end + end + context 'when a config file exists in the parent directory' do let(:dir_path) { 'dir' } From 469ca6147e96b9a7b8aac45cdd375c492505ad7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Sun, 12 Jul 2020 20:05:38 +0200 Subject: [PATCH 4/4] Add a changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b84785b30bf..c405d3d2646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ * [#8289](https://github.com/rubocop-hq/rubocop/issues/8289): Fix `Style/AccessorGrouping` to not register offense for accessor with comment. ([@tejasbubane][]) * [#8310](https://github.com/rubocop-hq/rubocop/pull/8310): Handle major version requirements in `Gemspec/RequiredRubyVersion`. ([@eugeneius][]) * [#8315](https://github.com/rubocop-hq/rubocop/pull/8315): Fix crash for `Style/PercentLiteralDelimiters` when the source contains invalid characters. ([@eugeneius][]) +* [#8239](https://github.com/rubocop-hq/rubocop/pull/8239): Don't load `.rubocop.yml` files at all outside of the current project, unless they are personal configuration files and the project has no configuration. ([@deivid-rodriguez][]) ### Changes