From d99f33b41a6c27b28272af7bdf1f3a58bd7f12fc Mon Sep 17 00:00:00 2001 From: Stephan Hagemann Date: Wed, 10 Aug 2022 21:10:24 -0600 Subject: [PATCH 1/5] Switch application load path finding to zeitwerk --- Gemfile.lock | 2 +- lib/packwerk/application_load_paths.rb | 2 +- packwerk.gemspec | 2 +- test/unit/application_load_paths_test.rb | 28 ++++++++++++++++++++++++ 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5552fb26b..fc3f9332d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -96,6 +96,7 @@ PATH parallel parser sorbet-runtime (>= 0.5.9914) + zeitwerk GEM remote: https://rubygems.org/ @@ -258,7 +259,6 @@ DEPENDENCIES sorbet-runtime spring tapioca - zeitwerk BUNDLED WITH 2.3.5 diff --git a/lib/packwerk/application_load_paths.rb b/lib/packwerk/application_load_paths.rb index 51c26b2d3..30ef1a736 100644 --- a/lib/packwerk/application_load_paths.rb +++ b/lib/packwerk/application_load_paths.rb @@ -20,7 +20,7 @@ def extract_relevant_paths(root, environment) sig { returns(T::Hash[String, Module]) } def extract_application_autoload_paths - Rails.autoloaders.inject({}) do |h, loader| + Zeitwerk::Registry.loaders.inject({}) do |h, loader| h.merge(loader.root_dirs) end end diff --git a/packwerk.gemspec b/packwerk.gemspec index 7cad32c06..327c8f1a7 100644 --- a/packwerk.gemspec +++ b/packwerk.gemspec @@ -45,13 +45,13 @@ Gem::Specification.new do |spec| spec.add_dependency("constant_resolver", ">=0.2.0") spec.add_dependency("parallel") spec.add_dependency("sorbet-runtime", ">=0.5.9914") + spec.add_dependency("zeitwerk") spec.add_development_dependency("m") spec.add_development_dependency("rake") spec.add_development_dependency("sorbet") # https://github.com/ruby/psych/pull/487 spec.add_development_dependency("psych", "~> 3") - spec.add_development_dependency("zeitwerk") # For Ruby parsing spec.add_dependency("ast") diff --git a/test/unit/application_load_paths_test.rb b/test/unit/application_load_paths_test.rb index 810bf3f22..5709ee8d2 100644 --- a/test/unit/application_load_paths_test.rb +++ b/test/unit/application_load_paths_test.rb @@ -50,5 +50,33 @@ class ApplicationLoadPathsTest < Minitest::Test ApplicationLoadPaths.extract_relevant_paths("/application", "test") end + + test ".extract_relevant_paths uses Zeitwerk loaders" do + result = ApplicationLoadPaths.extract_relevant_paths("./test/fixtures/skeleton", "test") + assert_equal([ + "components/sales/app/models", + "components/platform/app/models", + "components/timeline/app/models", + "components/timeline/app/models/concerns", + "vendor/cache/gems/example/models", + ], + result.keys + ) + + loader = Zeitwerk::Loader.new + loader.push_dir('./test/fixtures/skeleton/gems/gem_a') + + result = ApplicationLoadPaths.extract_relevant_paths("./test/fixtures/skeleton", "test") + assert_equal([ + "components/sales/app/models", + "components/platform/app/models", + "components/timeline/app/models", + "components/timeline/app/models/concerns", + "vendor/cache/gems/example/models", + "gems/gem_a", + ], + result.keys + ) + end end end From 9c2b13eed8ed51d92d714584d2b9cd8fa57a01a3 Mon Sep 17 00:00:00 2001 From: Stephan Hagemann Date: Wed, 10 Aug 2022 21:30:25 -0600 Subject: [PATCH 2/5] Add gitkeep for new gem folder in skeleton --- test/fixtures/skeleton/gems/gem_a/.gitkeep | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 test/fixtures/skeleton/gems/gem_a/.gitkeep diff --git a/test/fixtures/skeleton/gems/gem_a/.gitkeep b/test/fixtures/skeleton/gems/gem_a/.gitkeep new file mode 100644 index 000000000..e69de29bb From c8afec5be424df4fc3995725db3a917ea31af2cb Mon Sep 17 00:00:00 2001 From: Stephan Hagemann Date: Wed, 10 Aug 2022 21:37:26 -0600 Subject: [PATCH 3/5] Fix rubocop issues --- test/unit/application_load_paths_test.rb | 34 +++++++++++------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/test/unit/application_load_paths_test.rb b/test/unit/application_load_paths_test.rb index 5709ee8d2..5069d9358 100644 --- a/test/unit/application_load_paths_test.rb +++ b/test/unit/application_load_paths_test.rb @@ -54,29 +54,27 @@ class ApplicationLoadPathsTest < Minitest::Test test ".extract_relevant_paths uses Zeitwerk loaders" do result = ApplicationLoadPaths.extract_relevant_paths("./test/fixtures/skeleton", "test") assert_equal([ - "components/sales/app/models", - "components/platform/app/models", - "components/timeline/app/models", - "components/timeline/app/models/concerns", - "vendor/cache/gems/example/models", - ], - result.keys - ) + "components/sales/app/models", + "components/platform/app/models", + "components/timeline/app/models", + "components/timeline/app/models/concerns", + "vendor/cache/gems/example/models", + ], + result.keys) loader = Zeitwerk::Loader.new - loader.push_dir('./test/fixtures/skeleton/gems/gem_a') + loader.push_dir("./test/fixtures/skeleton/gems/gem_a") result = ApplicationLoadPaths.extract_relevant_paths("./test/fixtures/skeleton", "test") assert_equal([ - "components/sales/app/models", - "components/platform/app/models", - "components/timeline/app/models", - "components/timeline/app/models/concerns", - "vendor/cache/gems/example/models", - "gems/gem_a", - ], - result.keys - ) + "components/sales/app/models", + "components/platform/app/models", + "components/timeline/app/models", + "components/timeline/app/models/concerns", + "vendor/cache/gems/example/models", + "gems/gem_a", + ], + result.keys) end end end From a0ffecedfaec2a9b9aef2e9fd8d14d634db8dc52 Mon Sep 17 00:00:00 2001 From: Stephan Hagemann Date: Thu, 11 Aug 2022 08:13:12 -0600 Subject: [PATCH 4/5] =?UTF-8?q?=E2=80=9CFixed=E2=80=9D=20the=20readme=20;)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index ae09c3c58..b5771897b 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ We are keeping `packwerk` compatible with current versions of Ruby and Rails, bu -- _Sandi Metz, Practical Object-Oriented Design in Ruby_ -Packwerk is a Ruby gem used to enforce boundaries and modularize Rails applications. +Packwerk is a Ruby gem used to enforce boundaries and modularize Ruby applications (including Rails apps!). Packwerk can be used to: * Combine groups of files into packages From db2bfd02af08a1de2ef499925c78b6be755b014c Mon Sep 17 00:00:00 2001 From: Stephan Hagemann Date: Thu, 11 Aug 2022 08:40:11 -0600 Subject: [PATCH 5/5] =?UTF-8?q?Use=20zeitwerk=E2=80=99s=20public=20API=20f?= =?UTF-8?q?or=20finding=20app=20load=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/packwerk/application_load_paths.rb | 20 +++++++++----------- lib/packwerk/run_context.rb | 2 +- test/unit/application_load_paths_test.rb | 18 +++++++++--------- test/unit/cli_test.rb | 6 +++--- 4 files changed, 22 insertions(+), 24 deletions(-) diff --git a/lib/packwerk/application_load_paths.rb b/lib/packwerk/application_load_paths.rb index 30ef1a736..2d642cb76 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,28 @@ 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 - Zeitwerk::Registry.loaders.inject({}) do |h, loader| - h.merge(loader.root_dirs) - end + Zeitwerk::Loader.all_dirs 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 +57,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 5069d9358..e7ef3b012 100644 --- a/test/unit/application_load_paths_test.rb +++ b/test/unit/application_load_paths_test.rb @@ -11,41 +11,41 @@ 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 } + # paths = paths.each_with_object([]) { |p, h| h << p.to_s } 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 } + # 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") @@ -60,7 +60,7 @@ class ApplicationLoadPathsTest < Minitest::Test "components/timeline/app/models/concerns", "vendor/cache/gems/example/models", ], - result.keys) + result) loader = Zeitwerk::Loader.new loader.push_dir("./test/fixtures/skeleton/gems/gem_a") @@ -74,7 +74,7 @@ class ApplicationLoadPathsTest < Minitest::Test "vendor/cache/gems/example/models", "gems/gem_a", ], - result.keys) + result) end end end 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])