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

Support for multiple parsers #183

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

antstorm
Copy link
Contributor

Resurrecting a previous proposal — #164

This is basically a step 1 that allows multiple parsers to co-exist without breaking the default behaviour. A new parser is registered under a specified name and then can either be used explicitly by providing :parser option to the Monetize.parse or implicitly by setting the Monetize.default_parser.

The change here should be pretty straightforward, the bulk of it is moving the current parser to optimistic_parser.rb, reserving the parser.rb for the interface definition.

I've kept the diff as tight as possible, however I feel that a worthy addition would be to change the semantics of a parser class slightly. Right now it represents a parsing of a single input, meaning that a unique class gets created every time .parse is called. There aren't lots of benefits to this design besides easy access to the input and options supplied to the Monetize.parse (which creates some confusion in the current parser as input is mostly shadowed anyways). An alternative would be to treat the parser class as a pre-configured parser ready to parse a given input.

In the next PR I'll bring the StrictParser in, which will allow us to release it and gather feedback.

As a side note — given that monetize is mostly a parser with a few smaller additions I wonder if the strict parser should be a separate gem altogether assuming most people would want to use one parser across their codebases. This also allows for a super speedy strict parser gem written in Rust or C (which you don't want to impose on people already using monetize, because it adds installation time dependencies). We can discuss this later, just something to keep in mind.

This was referenced Aug 14, 2024
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.

1 participant