-
Notifications
You must be signed in to change notification settings - Fork 120
Allow fetching all references for a file, or all files, using public API #397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # typed: strict | ||
| # frozen_string_literal: true | ||
|
|
||
| module Packwerk | ||
| # 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 | ||
| 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 | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe T::Struct is slower than a normal struct. Please provide a benchmark, or switch to a plain old Struct (I think we only use Struct in other files).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 4 other uses of I did not think about performance differences and chose
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A quick benchmark shows that access is about the same, while instantiation is ~5x slower for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I guess you were looking for benchmarks of the whole thing, running packwerk on a real code base. Will do. I'm curious myself.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, on this codebase I'm currently looking at: 800k lines of Ruby according to There is no measurable difference between uncached I also tested with cache, and there is no measurable difference either. Packwerk reports ~3.6s, |
||
| 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) } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| Reference.new( | ||
| package: Package.new(name: package_name), | ||
| relative_path: package_name, | ||
| constant: ConstantContext.new, | ||
| source_location: nil | ||
| ) | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this the main method you use? Do we need to make the others public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
listis still useful to get references from a specific file.list_allgoes throughFilesForProcessingand thus respects includes and excludes, which could be confusing if you want to analyze a specific file that may be excluded.filesis public because it makes testing easier... I'm going to see how much more ugly it'd be to directly mockFilesForProcessing.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit that gets rid of the public
filesmethod in favour of some more elaborate stubbing in the test. It also renames the two remaining public methods so that hopefully their purpose is clearer.list->list_for_filelist_all->list_for_allThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh, I wonder now whether it makes sense to inject a FilesForProcessing instance in the test instead.