From 2bc81844e113f36668a19303df1e9200bb95fdf0 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Mon, 27 Nov 2023 20:30:26 -0600 Subject: [PATCH] Add associations_exclude to exclude files from association inspection Other libraries (like Active Model Serializers) use has_one / has_many macros like Active Record does. We don't need to pay attention to this. Instead of guessing what a node is based on what we can see in the file, we are using an exclude glob because it is too hard to know if a class is an Active Record or not without guessing. --- USAGE.md | 1 + lib/packwerk/association_inspector.rb | 21 ++++++++++++--- lib/packwerk/configuration.rb | 4 +++ lib/packwerk/const_node_inspector.rb | 4 +-- lib/packwerk/constant_name_inspector.rb | 4 +-- lib/packwerk/reference_extractor.rb | 6 ++++- lib/packwerk/run_context.rb | 19 ++++++++++--- .../packwerk/association_inspector_test.rb | 27 ++++++++++++++----- .../packwerk/const_node_inspector_test.rb | 14 +++++----- .../unit/packwerk/reference_extractor_test.rb | 2 +- 10 files changed, 74 insertions(+), 28 deletions(-) diff --git a/USAGE.md b/USAGE.md index 2b11677e8..b26255c13 100644 --- a/USAGE.md +++ b/USAGE.md @@ -77,6 +77,7 @@ Packwerk reads from the `packwerk.yml` configuration file in the root directory. |----------------------|-------------------------------------------|--------------| | include | **/*.{rb,rake,erb} | list of patterns for folder paths to include | | exclude | {bin,node_modules,script,tmp,vendor}/**/* | list of patterns for folder paths to exclude | +| associations_exclude | N/A | list of patterns for folder paths to exclude from association inspection | | package_paths | **/ | a single pattern or a list of patterns to find package configuration files, see: [Defining packages](#Defining-packages) | | custom_associations | N/A | list of custom associations, if any | | parallel | true | when true, fork code parsing out to subprocesses | diff --git a/lib/packwerk/association_inspector.rb b/lib/packwerk/association_inspector.rb index d2990b9fe..512962e14 100644 --- a/lib/packwerk/association_inspector.rb +++ b/lib/packwerk/association_inspector.rb @@ -19,19 +19,27 @@ class AssociationInspector CustomAssociations ) - sig { params(inflector: T.class_of(ActiveSupport::Inflector), custom_associations: CustomAssociations).void } - def initialize(inflector:, custom_associations: Set.new) + sig do + params( + inflector: T.class_of(ActiveSupport::Inflector), + custom_associations: CustomAssociations, + excluded_files: T::Set[String] + ).void + end + def initialize(inflector:, custom_associations: Set.new, excluded_files: Set.new) @inflector = inflector @associations = T.let(RAILS_ASSOCIATIONS + custom_associations, CustomAssociations) + @excluded_files = T.let(excluded_files, T::Set[String]) end sig do override - .params(node: AST::Node, ancestors: T::Array[AST::Node]) + .params(node: AST::Node, ancestors: T::Array[AST::Node], relative_file: String) .returns(T.nilable(String)) end - def constant_name_from_node(node, ancestors:) + def constant_name_from_node(node, ancestors:, relative_file:) return unless NodeHelpers.method_call?(node) + return if excluded?(relative_file) return unless association?(node) arguments = NodeHelpers.method_arguments(node) @@ -48,6 +56,11 @@ def constant_name_from_node(node, ancestors:) private + sig { params(relative_file: String).returns(T::Boolean) } + def excluded?(relative_file) + @excluded_files.include?(relative_file) + end + sig { params(node: AST::Node).returns(T::Boolean) } def association?(node) method_name = NodeHelpers.method_name(node) diff --git a/lib/packwerk/configuration.rb b/lib/packwerk/configuration.rb index 22a7dee20..7d106f8ed 100644 --- a/lib/packwerk/configuration.rb +++ b/lib/packwerk/configuration.rb @@ -54,6 +54,9 @@ def from_packwerk_config(path) sig { returns(T::Array[Symbol]) } attr_reader(:custom_associations) + sig { returns(T::Array[String]) } + attr_reader(:associations_exclude) + sig { returns(T.nilable(String)) } attr_reader(:config_path) @@ -76,6 +79,7 @@ def initialize(configs = {}, config_path: nil) @root_path = T.let(File.expand_path(root), String) @package_paths = T.let(configs["package_paths"] || "**/", T.any(String, T::Array[String])) @custom_associations = T.let((configs["custom_associations"] || []).map(&:to_sym), T::Array[Symbol]) + @associations_exclude = T.let(configs["associations_exclude"] || [], T::Array[String]) @parallel = T.let(configs.key?("parallel") ? configs["parallel"] : true, T::Boolean) @cache_enabled = T.let(configs.key?("cache") ? configs["cache"] : false, T::Boolean) @cache_directory = T.let(Pathname.new(configs["cache_directory"] || "tmp/cache/packwerk"), Pathname) diff --git a/lib/packwerk/const_node_inspector.rb b/lib/packwerk/const_node_inspector.rb index 8184d0535..6cbcda2d7 100644 --- a/lib/packwerk/const_node_inspector.rb +++ b/lib/packwerk/const_node_inspector.rb @@ -9,10 +9,10 @@ class ConstNodeInspector sig do override - .params(node: AST::Node, ancestors: T::Array[AST::Node]) + .params(node: AST::Node, ancestors: T::Array[AST::Node], relative_file: String) .returns(T.nilable(String)) end - def constant_name_from_node(node, ancestors:) + def constant_name_from_node(node, ancestors:, relative_file:) return nil unless NodeHelpers.constant?(node) parent = ancestors.first diff --git a/lib/packwerk/constant_name_inspector.rb b/lib/packwerk/constant_name_inspector.rb index 9acfa959e..16a95fe66 100644 --- a/lib/packwerk/constant_name_inspector.rb +++ b/lib/packwerk/constant_name_inspector.rb @@ -13,10 +13,10 @@ module ConstantNameInspector sig do abstract - .params(node: ::AST::Node, ancestors: T::Array[::AST::Node]) + .params(node: ::AST::Node, ancestors: T::Array[::AST::Node], relative_file: String) .returns(T.nilable(String)) end - def constant_name_from_node(node, ancestors:); end + def constant_name_from_node(node, ancestors:, relative_file:); end end private_constant :ConstantNameInspector diff --git a/lib/packwerk/reference_extractor.rb b/lib/packwerk/reference_extractor.rb index bc771c249..f5c08b56b 100644 --- a/lib/packwerk/reference_extractor.rb +++ b/lib/packwerk/reference_extractor.rb @@ -80,7 +80,11 @@ def reference_from_node(node, ancestors:, relative_file:) constant_name = T.let(nil, T.nilable(String)) @constant_name_inspectors.each do |inspector| - constant_name = inspector.constant_name_from_node(node, ancestors: ancestors) + constant_name = inspector.constant_name_from_node( + node, + ancestors: ancestors, + relative_file: relative_file + ) break if constant_name end diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 8dae554a1..d51c314d8 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -15,14 +15,13 @@ class << self params(configuration: Configuration).returns(RunContext) end def from_configuration(configuration) - inflector = ActiveSupport::Inflector - new( root_path: configuration.root_path, load_paths: configuration.load_paths, package_paths: configuration.package_paths, - inflector: inflector, + inflector: ActiveSupport::Inflector, custom_associations: configuration.custom_associations, + associations_exclude: configuration.associations_exclude, cache_enabled: configuration.cache_enabled?, cache_directory: configuration.cache_directory, config_path: configuration.config_path, @@ -39,6 +38,7 @@ def from_configuration(configuration) config_path: T.nilable(String), package_paths: T.nilable(T.any(T::Array[String], String)), custom_associations: AssociationInspector::CustomAssociations, + associations_exclude: T::Array[String], checkers: T::Array[Checker], cache_enabled: T::Boolean, ).void @@ -51,6 +51,7 @@ def initialize( config_path: nil, package_paths: nil, custom_associations: [], + associations_exclude: [], checkers: Checker.all, cache_enabled: false ) @@ -59,6 +60,7 @@ def initialize( @package_paths = package_paths @inflector = inflector @custom_associations = custom_associations + @associations_exclude = associations_exclude @checkers = checkers @cache_enabled = cache_enabled @cache_directory = cache_directory @@ -128,9 +130,18 @@ def resolver def constant_name_inspectors [ ConstNodeInspector.new, - AssociationInspector.new(inflector: @inflector, custom_associations: @custom_associations), + AssociationInspector.new( + inflector: @inflector, + custom_associations: @custom_associations, + excluded_files: relative_files_for_globs(@associations_exclude), + ), ] end + + sig { params(relative_globs: T::Array[String]).returns(FilesForProcessing::RelativeFileSet) } + def relative_files_for_globs(relative_globs) + Set.new(relative_globs.flat_map { |glob| Dir[glob] }) + end end private_constant :RunContext diff --git a/test/unit/packwerk/association_inspector_test.rb b/test/unit/packwerk/association_inspector_test.rb index 3acdebc26..93c0b93a4 100644 --- a/test/unit/packwerk/association_inspector_test.rb +++ b/test/unit/packwerk/association_inspector_test.rb @@ -14,49 +14,62 @@ class AssociationInspectorTest < Minitest::Test node = parse("has_lots :order") inspector = AssociationInspector.new(inflector: @inflector, custom_associations: [:has_lots]) - assert_equal "Order", inspector.constant_name_from_node(node, ancestors: []) + assert_equal "Order", inspector.constant_name_from_node(node, ancestors: [], relative_file: "") end test "finds target constant for simple association" do node = parse("has_one :order") inspector = AssociationInspector.new(inflector: @inflector) - assert_equal "Order", inspector.constant_name_from_node(node, ancestors: []) + assert_equal "Order", inspector.constant_name_from_node(node, ancestors: [], relative_file: "") end test "finds target constant for association that pluralizes" do node = parse("has_many :orders") inspector = AssociationInspector.new(inflector: @inflector) - assert_equal "Order", inspector.constant_name_from_node(node, ancestors: []) + assert_equal "Order", inspector.constant_name_from_node(node, ancestors: [], relative_file: "") end test "finds target constant for association if explicitly specified" do node = parse("has_one :cool_order, { class_name: 'Order' }") inspector = AssociationInspector.new(inflector: @inflector) - assert_equal "Order", inspector.constant_name_from_node(node, ancestors: []) + assert_equal "Order", inspector.constant_name_from_node(node, ancestors: [], relative_file: "") end test "rejects method calls that are not associations" do node = parse('puts "Hello World"') inspector = AssociationInspector.new(inflector: @inflector) - assert_nil inspector.constant_name_from_node(node, ancestors: []) + assert_nil inspector.constant_name_from_node(node, ancestors: [], relative_file: "") end test "gives up on metaprogrammed associations" do node = parse("has_one association_name") inspector = AssociationInspector.new(inflector: @inflector) - assert_nil inspector.constant_name_from_node(node, ancestors: []) + assert_nil inspector.constant_name_from_node(node, ancestors: [], relative_file: "") end test "gives up on dynamic class name" do node = parse("has_one :order, class_name: Order.name") inspector = AssociationInspector.new(inflector: @inflector) - assert_nil inspector.constant_name_from_node(node, ancestors: []) + assert_nil inspector.constant_name_from_node(node, ancestors: [], relative_file: "") + end + + test "gives up on excluded file node" do + node = parse("has_one :order") + inspector = AssociationInspector.new( + inflector: @inflector, excluded_files: Set["app/serializers/my_serializer.rb"] + ) + + assert_nil inspector.constant_name_from_node( + node, + ancestors: [], + relative_file: "app/serializers/my_serializer.rb", + ) end private diff --git a/test/unit/packwerk/const_node_inspector_test.rb b/test/unit/packwerk/const_node_inspector_test.rb index 15e7e29bf..c188a962d 100644 --- a/test/unit/packwerk/const_node_inspector_test.rb +++ b/test/unit/packwerk/const_node_inspector_test.rb @@ -13,7 +13,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase test "#constant_name_from_node should ignore any non-const nodes" do node = parse("a = 1 + 1") - constant_name = @inspector.constant_name_from_node(node, ancestors: []) + constant_name = @inspector.constant_name_from_node(node, ancestors: [], relative_file: "") assert_nil constant_name end @@ -21,7 +21,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase test "#constant_name_from_node should return correct name for const node" do node = parse("Order") - constant_name = @inspector.constant_name_from_node(node, ancestors: []) + constant_name = @inspector.constant_name_from_node(node, ancestors: [], relative_file: "") assert_equal "Order", constant_name end @@ -29,7 +29,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase test "#constant_name_from_node should return correct name for fully-qualified const node" do node = parse("::Order") - constant_name = @inspector.constant_name_from_node(node, ancestors: []) + constant_name = @inspector.constant_name_from_node(node, ancestors: [], relative_file: "") assert_equal "::Order", constant_name end @@ -37,7 +37,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase test "#constant_name_from_node should return correct name for compact const node" do node = parse("Sales::Order") - constant_name = @inspector.constant_name_from_node(node, ancestors: []) + constant_name = @inspector.constant_name_from_node(node, ancestors: [], relative_file: "") assert_equal "Sales::Order", constant_name end @@ -46,7 +46,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase parent = parse("class Order; end") node = NodeHelpers.each_child(parent).entries[0] - constant_name = @inspector.constant_name_from_node(node, ancestors: [parent]) + constant_name = @inspector.constant_name_from_node(node, ancestors: [parent], relative_file: "") assert_equal "::Order", constant_name end @@ -58,7 +58,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase ) # module node; second child is the body of the module node = NodeHelpers.each_child(parent).entries[0] # class node; first child is constant - constant_name = @inspector.constant_name_from_node(node, ancestors: [parent, grandparent]) + constant_name = @inspector.constant_name_from_node(node, ancestors: [parent, grandparent], relative_file: "") assert_equal "::Foo::Bar::Sales::Order", constant_name end @@ -68,7 +68,7 @@ class ConstNodeInspectorTest < ActiveSupport::TestCase parent = T.must(NodeHelpers.each_child(grandparent).entries[1]) # setup do self.class::HEADERS end node = NodeHelpers.each_child(parent).entries[2] # self.class::HEADERS - constant_name = @inspector.constant_name_from_node(node, ancestors: [parent, grandparent]) + constant_name = @inspector.constant_name_from_node(node, ancestors: [parent, grandparent], relative_file: "") assert_nil constant_name end diff --git a/test/unit/packwerk/reference_extractor_test.rb b/test/unit/packwerk/reference_extractor_test.rb index 077171f33..e0f1221de 100644 --- a/test/unit/packwerk/reference_extractor_test.rb +++ b/test/unit/packwerk/reference_extractor_test.rb @@ -222,7 +222,7 @@ def initialize(association: false, reference_name: "Dummy", expected_args: nil) @expected_args = expected_args end - def constant_name_from_node(node, ancestors:) + def constant_name_from_node(node, ancestors:, relative_file:) return nil unless @association return nil unless NodeHelpers.method_call?(node)