-
Notifications
You must be signed in to change notification settings - Fork 117
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
Allow fetching all references for a file, or all files, using public API #397
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 | ||
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). 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 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 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. 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 | ||
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
list
is still useful to get references from a specific file.list_all
goes throughFilesForProcessing
and thus respects includes and excludes, which could be confusing if you want to analyze a specific file that may be excluded.files
is public because it makes testing easier... I'm going to see how much more ugly it'd be to directly mockFilesForProcessing
.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
files
method 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_file
list_all
->list_for_all
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.
mmh, I wonder now whether it makes sense to inject a FilesForProcessing instance in the test instead.