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

Nom matching #26

Merged
merged 39 commits into from
Mar 20, 2022
Merged

Nom matching #26

merged 39 commits into from
Mar 20, 2022

Conversation

DrLuke
Copy link
Contributor

@DrLuke DrLuke commented Jan 18, 2022

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
Copy link
Owner

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 .

Copy link
Owner

@klingtnet klingtnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my first take on the implementation. Looks promising but we may need to optimize some parts.

src/address.rs Outdated Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Outdated Show resolved Hide resolved
src/address.rs Show resolved Hide resolved
DrLuke and others added 2 commits February 16, 2022 22:08
Co-authored-by: Andreas Linz <klingt.net@gmail.com>
Co-authored-by: Andreas Linz <klingt.net@gmail.com>
@DrLuke DrLuke changed the title WIP: Nom matching Nom matching Feb 16, 2022
@DrLuke
Copy link
Contributor Author

DrLuke commented Feb 16, 2022

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

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.

Copy link
Owner

@klingtnet klingtnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, good work and sorry for the delay 🙈

src/address.rs Outdated Show resolved Hide resolved
src/address.rs Show resolved Hide resolved
@klingtnet
Copy link
Owner

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
Copy link
Owner

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 #26 (comment). Thanks again for the contribution!

@klingtnet klingtnet merged commit 33f9522 into klingtnet:master Mar 20, 2022
@DrLuke DrLuke deleted the nom-matching branch March 20, 2022 10:08
@DrLuke
Copy link
Contributor Author

DrLuke commented Mar 20, 2022

Thank you for the great review!

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.

2 participants