From d29de0cf5c5234fc00987f1d8809f1d35934ccfe Mon Sep 17 00:00:00 2001 From: Stephan Hagemann Date: Thu, 11 Aug 2022 09:07:26 -0600 Subject: [PATCH] Simplify packwerk path determination mechanics --- lib/packwerk/application_load_paths.rb | 18 +++++++++--------- lib/packwerk/run_context.rb | 2 +- test/unit/application_load_paths_test.rb | 12 +++++------- test/unit/cli_test.rb | 6 +++--- 4 files changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/packwerk/application_load_paths.rb b/lib/packwerk/application_load_paths.rb index 51c26b2d3..51ac7c34b 100644 --- a/lib/packwerk/application_load_paths.rb +++ b/lib/packwerk/application_load_paths.rb @@ -9,7 +9,7 @@ module ApplicationLoadPaths class << self extend T::Sig - sig { params(root: String, environment: String).returns(T::Hash[String, Module]) } + sig { params(root: String, environment: String).returns(T::Array[String]) } def extract_relevant_paths(root, environment) require_application(root, environment) all_paths = extract_application_autoload_paths @@ -18,30 +18,30 @@ def extract_relevant_paths(root, environment) relative_path_strings(relevant_paths) end - sig { returns(T::Hash[String, Module]) } + sig { returns(T::Array[String]) } def extract_application_autoload_paths Rails.autoloaders.inject({}) do |h, loader| h.merge(loader.root_dirs) - end + end.keys end sig do - params(all_paths: T::Hash[String, Module], bundle_path: Pathname, rails_root: Pathname) - .returns(T::Hash[Pathname, Module]) + params(all_paths: T::Array[String], bundle_path: Pathname, rails_root: Pathname) + .returns(T::Array[Pathname]) end def filter_relevant_paths(all_paths, bundle_path: Bundler.bundle_path, rails_root: Rails.root) bundle_path_match = bundle_path.join("**") rails_root_match = rails_root.join("**") all_paths - .transform_keys { |path| Pathname.new(path).expand_path } + .map { |path| Pathname.new(path).expand_path } .select { |path| path.fnmatch(rails_root_match.to_s) } # path needs to be in application directory .reject { |path| path.fnmatch(bundle_path_match.to_s) } # reject paths from vendored gems end - sig { params(load_paths: T::Hash[Pathname, Module], rails_root: Pathname).returns(T::Hash[String, Module]) } + sig { params(load_paths: T::Array[Pathname], rails_root: Pathname).returns(T::Array[String]) } def relative_path_strings(load_paths, rails_root: Rails.root) - load_paths.transform_keys { |path| Pathname.new(path).relative_path_from(rails_root).to_s } + load_paths.map { |path| Pathname.new(path).relative_path_from(rails_root).to_s } end private @@ -59,7 +59,7 @@ def require_application(root, environment) end end - sig { params(paths: T::Hash[T.untyped, Module]).void } + sig { params(paths: T::Array[Pathname]).void } def assert_load_paths_present(paths) if paths.empty? raise <<~EOS diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 43a3b088c..29e514676 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -38,7 +38,7 @@ def from_configuration(configuration) sig do params( root_path: String, - load_paths: T::Hash[String, Module], + load_paths: T::Array[String], inflector: T.class_of(ActiveSupport::Inflector), cache_directory: Pathname, config_path: T.nilable(String), diff --git a/test/unit/application_load_paths_test.rb b/test/unit/application_load_paths_test.rb index 810bf3f22..e46a5130d 100644 --- a/test/unit/application_load_paths_test.rb +++ b/test/unit/application_load_paths_test.rb @@ -11,41 +11,39 @@ class ApplicationLoadPathsTest < Minitest::Test relative_path = "app/models" absolute_path = rails_root.join(relative_path) relative_path_strings = ApplicationLoadPaths.relative_path_strings( - { absolute_path => Object }, + [absolute_path], rails_root: rails_root ) - assert_equal({ relative_path => Object }, relative_path_strings) + assert_equal([relative_path], relative_path_strings) end test ".filter_relevant_paths excludes paths outside of the application root" do valid_paths = ["/application/app/models"] paths = valid_paths + ["/users/tobi/.gems/something/app/models", "/application/../something/"] - paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object } filtered_paths = ApplicationLoadPaths.filter_relevant_paths( paths, bundle_path: Pathname.new("/application/vendor/"), rails_root: Pathname.new("/application/") ) - assert_equal({ "/application/app/models" => Object }, filtered_paths.transform_keys(&:to_s)) + assert_equal(["/application/app/models"], filtered_paths.map(&:to_s)) end test ".filter_relevant_paths excludes paths from vendored gems" do valid_paths = ["/application/app/models"] paths = valid_paths + ["/application/vendor/something/app/models"] - paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object } filtered_paths = ApplicationLoadPaths.filter_relevant_paths( paths, bundle_path: Pathname.new("/application/vendor/"), rails_root: Pathname.new("/application/") ) - assert_equal({ "/application/app/models" => Object }, filtered_paths.transform_keys(&:to_s)) + assert_equal(["/application/app/models"], filtered_paths.map(&:to_s)) end test ".extract_relevant_paths calls out to filter the paths" do - ApplicationLoadPaths.expects(:filter_relevant_paths).once.returns(Pathname.new("/fake_path").to_s => Object) + ApplicationLoadPaths.expects(:filter_relevant_paths).once.returns([Pathname.new("/fake_path").to_s]) ApplicationLoadPaths.expects(:require_application).with("/application", "test").once.returns(true) ApplicationLoadPaths.extract_relevant_paths("/application", "test") diff --git a/test/unit/cli_test.rb b/test/unit/cli_test.rb index ec5ee270a..18bf71635 100644 --- a/test/unit/cli_test.rb +++ b/test/unit/cli_test.rb @@ -24,7 +24,7 @@ class CliTest < Minitest::Test offense = Offense.new(file: file_path, message: violation_message) configuration = Configuration.new({ "parallel" => false }) - configuration.stubs(load_paths: {}) + configuration.stubs(load_paths: []) RunContext.any_instance.stubs(:process_file).at_least_once.returns([offense]) string_io = StringIO.new @@ -49,7 +49,7 @@ class CliTest < Minitest::Test offense = Offense.new(file: file_path, message: violation_message) configuration = Configuration.new({ "parallel" => false }) - configuration.stubs(load_paths: {}) + configuration.stubs(load_paths: []) RunContext.any_instance.stubs(:process_file) .at_least(2) @@ -137,7 +137,7 @@ def show_stale_violations(_offense_collection, _fileset) offense = Offense.new(file: file_path, message: violation_message) configuration = Configuration.new - configuration.stubs(load_paths: {}) + configuration.stubs(load_paths: []) RunContext.any_instance.stubs(:process_file) .returns([offense])