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

Conversation

kenzan100
Copy link
Contributor

What are you trying to accomplish?

Trying to find a seam within the Packwerk module.

We have some rough exploration of how it works (see below diagram). This PR will acts as a self-annotated PR to invite more discussions on what module structure Packwerk should enjoy.

Responsibilities and its dependencies

Screen Shot 2021-10-13 at 12 55 52 PM

Data/object transformation journey

Screen Shot 2021-10-13 at 12 55 58 PM

What approach did you choose and why?

A couple of motivations to do this;

  • We're interested in what kind of contracts (interface) Packwerk can and should expose, so that 3rd party integration becomes possible. (checkers?)
  • We're also interested in documenting the high-level overview of "how it works", much like explained in https://matklad.github.io/2021/02/06/ARCHITECTURE.md.html

What should reviewers focus on?

Currently, the files completely ignore the proposed namespace. No need to commit editing each file until we're comfortable finalizing the proposed change.

  • Does a file belong to an appropriate directory?
  • Naming of a directory (namespace) sounds right to you?
  • What should be the pattern to bridge these seams? (how to interact/orchestrate between them)

Type of Change

Discussion only. The merge-able artifact should be created as a separate PR.

Comment on lines 57 to 81
# 1. file path to node
# 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

# 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.

Comment on lines +1 to +19
# Packwerk::Checkers

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

### 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.
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

@@ -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

@@ -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.

@@ -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.

@@ -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.

@@ -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

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?

@exterm
Copy link
Contributor

exterm commented Oct 13, 2021

I think in general this makes sense but we can talk more tomorrow. I think there'll be more interesting questions coming up when you start putting it together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants