Skip to content
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

[Discussion] Put files into buckets of responsibilities/seams #145

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 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
19 changes: 19 additions & 0 deletions lib/packwerk/checking_references/checkers/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Packwerk::Checkers

- Checker implements `invalid_reference?(reference)`, which takes a `Packwerk::Reference` and returns Boolean.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This is accurate, but it duplicates the interface definition in Packwerk::Checker, so I think in the final docs we should just point out that the interface exists instead of putting the details here


### Packwerk::Reference

- It contains the information about source package and destination package.


## Example: DependencyChecker

- Dependency means any outgoing dependency from the source package.
- If source package is enforcing dependency, it will check if declared dependency includes the destination package.


## Example: PrivacyChecker

- Privacy means any incoming dependency towards the destination package.
- If destination package is enforcing privacy, it till check if source package references non-public constants.
Comment on lines +1 to +19
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checker is an obvious candidate of what we can accept as 3rd party integration.

Does current implementation leak too much of Reference and subsequent objects?
For example, relative_path in Reference doesn't seem to be used at all, but I might be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think relative_path is used in, or through, the offense

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
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
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
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
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
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
Copy link
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
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
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