Configurable parsers (continued)

[richardmarbach]

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

Due to stalled activity in #243, I took liberty and completed the change requests from #243 (comment)
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
• New feature
• Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

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

[richardmarbach]

I have signed the CLA!

[richardmarbach]

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]

@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: euglena1215@4b722f2

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 #375 (comment).

Reference: euglena1215@73a79b3

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

[richardmarbach]

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

[euglena1215]

Please let me help get this pull request merged!
I have suggested several fixes, but I'm not a committer. So I'll leave the revision policy to you.

[euglena1215]

Hi @gmcgibbon !
I am hoping that this pull request will be merged so that YARD can do dependency checking by method argument/return value. #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]

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?

[Catsuko]

Not sure who the active maintainers are but based on PR history, @gmcgibbon or @nvasilevski perhaps

Any chance of merging this? As with a lot of the other devs here, I also want to use these changes to add a slim parser 🙏 🚀