Skip to content
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
File renamed without changes.
3 changes: 3 additions & 0 deletions lib/packwerk/configurations/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
### Configurations

Includes validators which are outside of primary `check` runs.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File renamed without changes.
4 changes: 4 additions & 0 deletions lib/packwerk/extracting_references/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Extracting references

- Given a `AST::Node`, this module extracts `Packwerk::Reference`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you put it like this, it sounds like everything here works in the context of a single node. However, ParsedConstantDefinitions works on a whole syntax tree.

- `Packwerk::Reference` contains `constant` information, that are worked by `Inflector`.
3 changes: 3 additions & 0 deletions lib/packwerk/file_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require "ast/node"

module Packwerk
# from file path to node
class FileProcessor
class UnknownFileTypeResult < Offense
def initialize(file:)
Expand All @@ -20,6 +21,7 @@ def call(file_path)
parser = @parser_factory.for_path(file_path)
return [UnknownFileTypeResult.new(file: file_path)] if parser.nil?

# path to node
node = File.open(file_path, "r", external_encoding: Encoding::UTF_8) do |file|
parser.call(io: file, file_path: file_path)
rescue Parsers::ParseError => e
Expand All @@ -31,6 +33,7 @@ def call(file_path)
node_processor = @node_processor_factory.for(filename: file_path, node: node)
node_visitor = Packwerk::NodeVisitor.new(node_processor: node_processor)

# node to offence
node_visitor.visit(node, ancestors: [], result: result)
Copy link
Copy Markdown
Contributor

@exterm exterm Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the content of this conditional doesn't really seem to fit here. I'd rather just hand off to something that processes the AST instead of instantiating processors and visitors here.

end
result
Expand Down
1 change: 1 addition & 0 deletions lib/packwerk/node_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# frozen_string_literal: true

module Packwerk
# from node to reference to offence
class NodeProcessor
extend T::Sig

Expand Down
File renamed without changes.
29 changes: 28 additions & 1 deletion lib/packwerk/run_context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
require "constant_resolver"

module Packwerk
# Packwerk used to run as a part of Rubocop.
# Now that Packwerk is a standalone script, this structure shouldn't be necessary.
class RunContext
extend T::Sig

Expand Down Expand Up @@ -52,7 +54,32 @@ def initialize(

sig { params(file: String).returns(T::Array[T.nilable(::Packwerk::Offense)]) }
def process_file(file:)
file_processor.call(file)
# 1. file path to node
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's more like file path to AST, but that contains nodes, so... you're not wrong

# It needs to return ancestors relative to node
node, ancestors = file_processor.call(file)

# Inside NodeProcessor - @reference_extractor.reference_from_node(node, ancestors: ancestors, file_path: @filename)
# 2. node to constant
# 3. constant to reference
@constant_name_inspectors.each do |inspector|
constant_name = inspector.constant_name_from_node(node, ancestors: ancestors)
break if constant_name
end

reference_from_constant(constant_name, node: node, ancestors: ancestors, file_path: file_path) if constant_name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah maybe at this point we'd have a list of references?


# Inside NodeProcessor
# 4. reference to an offence
@checkers.each_with_object([]) do |checker, violations|
next unless checker.invalid_reference?(reference)
offense = Packwerk::ReferenceOffense.new(
location: Node.location(node),
reference: reference,
violation_type: checker.violation_type
)
violations << offense
end
Comment on lines +57 to +97
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we will divide code's responsibility by their explicit "steps", something needs to have the responsibility of orchestration.

RunContext is just one convenient place atm, but what's more important is to find out if this way of separation of responsibility is what we want to go for, and whether or not the interface between each step is clean and extensible.

Does this direction make sense to you guys? @exterm @dougedey-shopify

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we eliminate RunContext and let ParseRun orchestrate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, certainly! I'll first focus on isolating the responsibilities of data transformation within the RunContext, then will follow up with the PR to eliminate the redundancy between RunContext and ParseRun.


end

private
Expand Down