Configurable parsers (continued)

richardmarbach commented 9 months ago

Co-authored-by: Davis Namsons davisnamsons@gmail.com

Due to stalled activity in #243, I took liberty and completed the change requests from https://github.com/Shopify/packwerk/pull/243#issuecomment-1339428760 Due to messy merge conflicts, I decided to redo the feature with a fresh commit history. I co-authored @dnamsons, as he did most of the hard work!

This is the first time I've used minitest, so I'd love some feedback on my usage. The rest of the PR body is a copy of the original.

What are you trying to accomplish?

As discussed on #142, this change allows for additional parsers to be added.

This also allows extracting the erb parser out of packwerk(which might be desired - #142 (comment)).

What approach did you choose and why?

modified the Packwerk::Parsers::Factory class to store parsers in an instance variable and made this variable modifiable via the use of an attr_accessor.
moved the regex constants into their respective parser classes and added a new method .match? to each parser class which verifies if a given path is relevant to the parser.
changed Packwerk::Parsers::Factory#for_path to iterate through the defined parsers to find a parser whose match? method returns true.

I considered multiple different alternatives to how new parsers can be defined(and the existing ones removed) but could not land on a satisfactory solution, so I stuck with the simplest one in hopes I could get some advice on how best to proceed.

Currently every parser is initialized anew for every new file that matches with its regex. I considered either storing already initialized parser instances in the parsers instance variable or, alternatively, creating a sort of a "cache" of parser instances but wasn't sure on which would be preferable.

What should reviewers focus on?

As mentioned above, my current approach to the the configuration of parsers is simplistic, I would appreciate any input on how to make this be more in line with the general structure of packwerk.

Type of Change

[ ] Bugfix
[x] New feature
[ ] Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

[x] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] I have updated the documentation accordingly.
[x] I have added tests to cover my changes.
[x] It is safe to rollback this change.

richardmarbach commented 9 months ago

I have signed the CLA!

richardmarbach commented 8 months ago

I had some tests failing sporadically, so I've managed to surface three different failure modes with the seeds: 37226, 30561, and 17743.

I still need to track down the root cause, but the default parsers aren't registered in seeds 37226 and 17743, so I suspect something with the autoloading. The seed 30561 seems to be related to the caching. Right now I suspect one of the specs isn't clearing it properly.

Unfortunately, I haven't had time to dig into it. Hopefully I'll get to it on the weekend

euglena1215 commented 6 months ago

@richardmarbach Hi, I am using this pull request to create a gem to parse YARD. In the process of creating it, I found two things that I would like to fix.

1. Remove `ParserInterface` from autoload target

The ParserInterface class was removed in this pull request, so it needs to be removed from the autoload target.

Reference: https://github.com/euglena1215/packwerk/pull/1/commits/4b722f2c66c5379f35626494424c37960adb7082

2. require each parser class before executing `Packwerk::FileParser.all`

Since each parser class is implemented to be loaded by autoload, it is not registered in the @parsers instance variable of the Packwerk::FileParser class unless you explicitly reference the constant or require it. As a fact, when I ran packwerk with this branch, it did not perform dependency checks on the ERB file.

I can't see the CI results, so I don't know the details, but I hope this fix will fix https://github.com/Shopify/packwerk/pull/375#issuecomment-1777772726.

Reference: https://github.com/euglena1215/packwerk/pull/1/commits/73a79b30fb11df1867dffca8c2485aed0e06a837

That is all. I look forward to this pull request being merged!

richardmarbach commented 6 months ago

Thanks for looking into it, @euglena1215! Those fixes resolved the outstanding issues with the tests.

euglena1215 commented 6 months ago

Hi @gmcgibbon ! I am hoping that this pull request will be merged so that YARD can do dependency checking by method argument/return value. https://github.com/Shopify/packwerk/issues/383 That way we can make better use of packwerk in our project.

Is there any information missing to review this pull request? I would like to provide that support!

timlkelly commented 3 months ago

I'm also interested in using the Packwerk HAML plugin! Is there anything else needed for this pull request? or is it just waiting for review?

Shopify / packwerk