Refactor - simplify orchestration layer #147
Conversation
| T::Array[ | ||
| T.any( | ||
| Packwerk::Reference, | ||
| Packwerk::Offense, |
There was a problem hiding this comment.
FileProcessor still have chance to early-return Offence, in cases of UnknownFileTypeResult or ParseError.
I don't particularly like this shared responsibility of the class, and more than happy to revisit the further separation.
| ) | ||
| violations << offense | ||
| end | ||
| def call(node, ancestors) |
There was a problem hiding this comment.
Result of this change, identifies further refactoring opportunity;
NodeProcessor and its factory is becoming almost negligible wrapper on reference_extractor. To be worked on as a follow-up of this PR.
| params( | ||
| node: Parser::AST::Node, | ||
| ancestors: T::Array[Parser::AST::Node] | ||
| ).returns(T.nilable(Packwerk::Reference)) |
There was a problem hiding this comment.
❤️
sharpening the definition of the node processor to only generate references - I love it
|
Really cool refactor. It all makes sense to me. When you run this in core CI, could you also check the run times to make sure it isn't dramatically slower than the latest release? |
I've create two branches in core CI, they're identical except for which branch/git source it uses for Packwerk. I don't think I'm doing anything obviously harmful for the Packwerk run-time. (And ofc, all the packwerk scan passed on this branch 🎉) |
kenzan100
changed the title
|
I don't think 1-2% are statistically significant, so all good |
| end | ||
|
|
||
| def parser_for(file_path) | ||
| @parser_factory.for_path(file_path) | ||
| end |
There was a problem hiding this comment.
this class looks so much better now.
|
|
||
| module Packwerk | ||
| module ReferenceToOffense | ||
| class ReferenceChecker |
There was a problem hiding this comment.
It's a bit confusing that ReferenceChecker has a very similar name to PrivacyChecker and DependencyChecker yet is not a Checker.
I can't think of a good alternative name right now though...
|
Just the two names left that I'm not sure about, but I think this is otherwise ready to merge. |
Changelog entry for a refactor? If it needs a changelog entry it is not a refactor. |
I've looked into https://github.com/Shopify/packwerk/blob/main/USAGE.md, but none of the change here, would affect public facing (a bit ambiguous term) functionality. |
|
LGTM, but you will need to clean those commits. Can you please squash/rename the commits so they makes sense? |
- Extract checkers into its own namespace / module. - Decouple reference extraction from offence checks.
kenzan100
force-pushed
the
yuta/simplify-orchestration
branch
from
cc3942f
to
93d1c28
Compare
|
🎉 thank you so much for the reviewers. |
What are you trying to accomplish?
First review-ready PR as an artefact of #145.
It's a part of Packwerk upkeep attempts.
What approach did you choose and why?
1. Extract checkers into its own module.
Biggest extractable seam/interface seems to be the implementation of
Checkers: We have two default checkers:DependencyCheckerandPrivacyChecker. They're extracted into its own namespace.2. Decouple reference extraction from offence checks.
FileProcessorwas having too much responsibilities, of converting file_path, into AST nodes, into a list of offences.Now seams are clearer;
file_path-(AST::Node)->References-(ReferenceChecker)->Offences.What should reviewers focus on?
There're self-annotated comments in the diff for one-off topics.
Type of Change
Additional Release Notes
Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.
If no additional notes are necessary, delete this section or leave it unchanged.
Checklist