From 49f57d1c76d8e0068e2780f57c4f2d9336f9e39a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philip=20M=C3=BCller?= Date: Mon, 18 Mar 2024 18:37:56 +0000 Subject: [PATCH 1/4] hacky MVP --- lib/packwerk/references_from_file.rb | 35 ++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 lib/packwerk/references_from_file.rb diff --git a/lib/packwerk/references_from_file.rb b/lib/packwerk/references_from_file.rb new file mode 100644 index 000000000..85da18473 --- /dev/null +++ b/lib/packwerk/references_from_file.rb @@ -0,0 +1,35 @@ +# typed: false +# frozen_string_literal: true + +module Packwerk + # Extracts all static constant references between Ruby files. + # Reuses the packwerk configuration. + # Hackishly hooks into packwerk internals. + class ReferencesFromFile + def initialize(config = Configuration.from_path) + @config = config + # RunContext is a `private_constant` of `Packwerk` + @run_context = RunContext.from_configuration(@config) + end + + def list_all(relative_file_paths: []) + # FilesForProcessing is a `private_constant` of `Packwerk` + files = FilesForProcessing.fetch(relative_file_paths:, configuration: @config).files + + files.map { |file| list(file) }.flatten(1) + end + + def list(relative_file) + file_processor = @run_context.send(:file_processor) + context_provider = @run_context.send(:context_provider) + + unresolved_references = file_processor.call(relative_file).unresolved_references + + # ReferenceExtractor is a `private_constant` of `Packwerk` + ReferenceExtractor.get_fully_qualified_references_from( + unresolved_references, + context_provider + ) + end + end + end \ No newline at end of file From f5fdbd33a1c149c12173b95c79e08bb1fd398ae2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philip=20M=C3=BCller?= Date: Mon, 18 Mar 2024 18:58:33 +0000 Subject: [PATCH 2/4] proposed cleaner solution --- lib/packwerk.rb | 1 + lib/packwerk/references_from_file.rb | 68 ++++++++++++++++------------ lib/packwerk/run_context.rb | 18 +++++++- 3 files changed, 56 insertions(+), 31 deletions(-) diff --git a/lib/packwerk.rb b/lib/packwerk.rb index 39023d1ca..765de3efe 100644 --- a/lib/packwerk.rb +++ b/lib/packwerk.rb @@ -32,6 +32,7 @@ module Packwerk autoload :Reference autoload :ReferenceOffense autoload :Validator + autoload :ReferencesFromFile module OutputStyles extend ActiveSupport::Autoload diff --git a/lib/packwerk/references_from_file.rb b/lib/packwerk/references_from_file.rb index 85da18473..e8e3ede6a 100644 --- a/lib/packwerk/references_from_file.rb +++ b/lib/packwerk/references_from_file.rb @@ -1,35 +1,45 @@ -# typed: false +# typed: strict # frozen_string_literal: true module Packwerk - # Extracts all static constant references between Ruby files. - # Reuses the packwerk configuration. - # Hackishly hooks into packwerk internals. - class ReferencesFromFile - def initialize(config = Configuration.from_path) - @config = config - # RunContext is a `private_constant` of `Packwerk` - @run_context = RunContext.from_configuration(@config) - end - - def list_all(relative_file_paths: []) - # FilesForProcessing is a `private_constant` of `Packwerk` - files = FilesForProcessing.fetch(relative_file_paths:, configuration: @config).files - - files.map { |file| list(file) }.flatten(1) + # Extracts all static constant references between Ruby files. + class ReferencesFromFile + extend T::Sig + + class FileParserError < RuntimeError + extend T::Sig + + sig { params(file: String, offenses: T::Array[Packwerk::Offense]).void } + def initialize(file:, offenses:) + super("Errors while parsing #{file}: #{offenses.map(&:to_s).join("\n")}") end - - def list(relative_file) - file_processor = @run_context.send(:file_processor) - context_provider = @run_context.send(:context_provider) - - unresolved_references = file_processor.call(relative_file).unresolved_references - - # ReferenceExtractor is a `private_constant` of `Packwerk` - ReferenceExtractor.get_fully_qualified_references_from( - unresolved_references, - context_provider - ) + end + + sig { params(config: Packwerk::Configuration).void } + def initialize(config = Configuration.from_path) + @config = config + @run_context = T.let(RunContext.from_configuration(@config), RunContext) + end + + sig { params(relative_file_paths: T::Array[String]).returns(T::Array[Packwerk::Reference]) } + def list_all(relative_file_paths: []) + files(relative_file_paths: relative_file_paths).flat_map { |file| list(file) } + end + + sig { params(relative_file: String).returns(T::Array[Packwerk::Reference]) } + def list(relative_file) + references_result = @run_context.references_from_file(relative_file: relative_file) + + if references_result.file_offenses.present? + raise FileParserError.new(file: relative_file, offenses: references_result.file_offenses) end + + references_result.references + end + + sig { params(relative_file_paths: T::Array[String]).returns(T::Set[String]) } + def files(relative_file_paths: []) + FilesForProcessing.fetch(relative_file_paths: relative_file_paths, configuration: @config).files end - end \ No newline at end of file + end +end diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 8dae554a1..636a016ac 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -75,15 +75,29 @@ def initialize( sig { params(relative_file: String).returns(T::Array[Packwerk::Offense]) } def process_file(relative_file:) + reference_checker = ReferenceChecking::ReferenceChecker.new(@checkers) + + references_result = references_from_file(relative_file: relative_file) + + references_result.file_offenses + + references_result.references.flat_map { |reference| reference_checker.call(reference) } + end + + class FileReferencesResult < T::Struct + const :references, T::Array[Packwerk::Reference] + const :file_offenses, T::Array[Packwerk::Offense] + end + + sig { params(relative_file: String).returns(FileReferencesResult) } + def references_from_file(relative_file:) processed_file = file_processor.call(relative_file) references = ReferenceExtractor.get_fully_qualified_references_from( processed_file.unresolved_references, context_provider ) - reference_checker = ReferenceChecking::ReferenceChecker.new(@checkers) - processed_file.offenses + references.flat_map { |reference| reference_checker.call(reference) } + FileReferencesResult.new(references: references, file_offenses: processed_file.offenses) end sig { returns(PackageSet) } From 028bd25cde708f481588bf26f07be2361bd0fcc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philip=20M=C3=BCller?= Date: Tue, 19 Mar 2024 16:03:45 +0000 Subject: [PATCH 3/4] add tests --- .../packwerk/references_from_file_test.rb | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 test/unit/packwerk/references_from_file_test.rb diff --git a/test/unit/packwerk/references_from_file_test.rb b/test/unit/packwerk/references_from_file_test.rb new file mode 100644 index 000000000..cde52d511 --- /dev/null +++ b/test/unit/packwerk/references_from_file_test.rb @@ -0,0 +1,59 @@ +# typed: true +# frozen_string_literal: true + +require "test_helper" + +module Packwerk + class ReferencesFromFileTest < Minitest::Test + setup do + config = Configuration.new + config.stubs(:load_paths).returns({}) + @run_context = RunContext.from_configuration(config) + RunContext.stubs(:from_configuration).with(config).returns(@run_context) + @referencer = ReferencesFromFile.new(config) + end + + test "raises on parser error" do + offense = Offense.new(file: "something.rb", message: "yo") + @run_context.stubs(:references_from_file).returns( + RunContext::FileReferencesResult.new(file_offenses: [offense], references: []) + ) + + assert_raises ReferencesFromFile::FileParserError do + @referencer.list("lib/something.rb") + end + end + + test "fetches violations for all files from run context" do + references = { + "lib/something.rb" => [ + make_fake_reference, + ], + "components/ice_cream_sales/app/models/scoop.rb" => [ + make_fake_reference, + ], + } + @referencer.stubs(:files).returns(references.keys) + + references.each do |file, references| + @run_context.stubs(:references_from_file).with(relative_file: file).returns( + RunContext::FileReferencesResult.new(file_offenses: [], references: references) + ) + end + + assert_equal Set.new(references.values.flatten), Set.new(@referencer.list_all) + end + + private + + def make_fake_reference + package_name = Array("ilikeletters".chars.sample(5)).join + Reference.new( + package: Package.new(name: package_name), + relative_path: package_name, + constant: ConstantContext.new, + source_location: nil + ) + end + end +end From e7013a6e4dd31dfbfba37344841e24f17da2e8c4 Mon Sep 17 00:00:00 2001 From: Philip Mueller Date: Mon, 15 Jul 2024 14:22:34 -0400 Subject: [PATCH 4/4] refactor and clarify API --- lib/packwerk/references_from_file.rb | 12 ++++-------- .../unit/packwerk/references_from_file_test.rb | 18 ++++++++++-------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/lib/packwerk/references_from_file.rb b/lib/packwerk/references_from_file.rb index e8e3ede6a..e516304c5 100644 --- a/lib/packwerk/references_from_file.rb +++ b/lib/packwerk/references_from_file.rb @@ -22,12 +22,13 @@ def initialize(config = Configuration.from_path) end sig { params(relative_file_paths: T::Array[String]).returns(T::Array[Packwerk::Reference]) } - def list_all(relative_file_paths: []) - files(relative_file_paths: relative_file_paths).flat_map { |file| list(file) } + def list_for_all(relative_file_paths: []) + files = FilesForProcessing.fetch(relative_file_paths: relative_file_paths, configuration: @config).files + files.flat_map { |file| list_for_file(file) } end sig { params(relative_file: String).returns(T::Array[Packwerk::Reference]) } - def list(relative_file) + def list_for_file(relative_file) references_result = @run_context.references_from_file(relative_file: relative_file) if references_result.file_offenses.present? @@ -36,10 +37,5 @@ def list(relative_file) references_result.references end - - sig { params(relative_file_paths: T::Array[String]).returns(T::Set[String]) } - def files(relative_file_paths: []) - FilesForProcessing.fetch(relative_file_paths: relative_file_paths, configuration: @config).files - end end end diff --git a/test/unit/packwerk/references_from_file_test.rb b/test/unit/packwerk/references_from_file_test.rb index cde52d511..531fa5b59 100644 --- a/test/unit/packwerk/references_from_file_test.rb +++ b/test/unit/packwerk/references_from_file_test.rb @@ -6,11 +6,11 @@ module Packwerk class ReferencesFromFileTest < Minitest::Test setup do - config = Configuration.new - config.stubs(:load_paths).returns({}) - @run_context = RunContext.from_configuration(config) - RunContext.stubs(:from_configuration).with(config).returns(@run_context) - @referencer = ReferencesFromFile.new(config) + @config = Configuration.new + @config.stubs(:load_paths).returns({}) + @run_context = RunContext.from_configuration(@config) + RunContext.stubs(:from_configuration).with(@config).returns(@run_context) + @referencer = ReferencesFromFile.new(@config) end test "raises on parser error" do @@ -20,7 +20,7 @@ class ReferencesFromFileTest < Minitest::Test ) assert_raises ReferencesFromFile::FileParserError do - @referencer.list("lib/something.rb") + @referencer.list_for_file("lib/something.rb") end end @@ -33,7 +33,9 @@ class ReferencesFromFileTest < Minitest::Test make_fake_reference, ], } - @referencer.stubs(:files).returns(references.keys) + ffp_mock = mock("FilesForProcessing instance") + ffp_mock.stubs(:files).returns(references.keys) + FilesForProcessing.stubs(:fetch).with(relative_file_paths: [], configuration: @config).returns(ffp_mock) references.each do |file, references| @run_context.stubs(:references_from_file).with(relative_file: file).returns( @@ -41,7 +43,7 @@ class ReferencesFromFileTest < Minitest::Test ) end - assert_equal Set.new(references.values.flatten), Set.new(@referencer.list_all) + assert_equal Set.new(references.values.flatten), Set.new(@referencer.list_for_all) end private