search

klingtnet / rosc

An OSC library for Rust.
Apache License 2.0
173 stars 25 forks source link

Nom matching #26

Closed DrLuke closed 2 years ago

DrLuke commented 2 years ago

This is my attempt at implementing a nom-based matcher to get rid of regex dependency as explained in #23.

It works by parsing the address pattern and separating it into AddressPatternComponents. When matching, the matching method iterates over the components and applies the correct nom parser for each component, consuming the address to be matched part by part. If everything is consumed and every component parser was run, the match is successful, otherwise it's a failure.

The one tricky thing to implement were wildcards in combination with other elements (e.g. /foo/*{bar,baz}andsometext). The wildcard parser was implemented to be greedy, so it tries to consume as many characters as possible.

This is still early WIP. The matcher is fully implemented, but everything around it needs work (documentation, removing unused functions, etc.), but I'd be happy about comments.

klingtnet commented 2 years ago

Just a quick life sign, I will try to review the implementation somewhen next week. I am already pretty happy to see nom matching getting implemented since this should resolve #23 .

DrLuke commented 2 years ago

As far as I'm concerned this can be merged (once the remaining conversations are resolved).

Some (potential) follow-up TODOs:

klingtnet commented 2 years ago

As far as I'm concerned this can be merged (once the remaining conversations are resolved).

From my side as well, only thing, the range flip, should be removed in my opinion.

Some (potential) follow-up TODOs:

* We need a struct for addresses so we only have to validate them once instead of every time `match_address` is invoked

* Optimize away the `remainder` in `match_address`

* nom is capable of reporting where exactly matching failed, and for what reason. I'm not sure if this feature is necessary though.

All the potential follow-ups are reasonable and should be defined in GitHub issues.

klingtnet commented 2 years ago

I haven't noticed that you now fail on reversed character ranges otherwise I would have merged this already. Anyhow, I created three follow-up issue based on the optimizations of https://github.com/klingtnet/rosc/pull/26#issuecomment-1042364232. Thanks again for the contribution!

DrLuke commented 2 years ago

Thank you for the great review!