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

Abstract from Regex in pattern validator and add support for regex-lite #57

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

jirutka
Copy link
Contributor

@jirutka jirutka commented Jul 29, 2023

The last commit makes regex-lite an equivalent alternative to regex and allows to drop dependency on the regex crate when regex-lite is enabled. However, since Cargo features are awfully limited and clumsy, it’s quite ugly. I don’t suppose you would accept such change (the last commit), so I leave it here only as a demonstration.

I believe it’s quite useful even without the ability to avoid pulling regex into the dependency graph, because if it’s not used (i.e. the user avoids using the pattern validator with string literals), the compiler will remove it when using LTO.

@jirutka jirutka force-pushed the regex-trait branch 2 times, most recently from f613346 to eca45ae Compare July 29, 2023 20:27
@jprochazk
Copy link
Owner

jprochazk commented Jul 31, 2023

Sorry, haven't had time to properly look at this until now.

My immediate reaction is that I'm not sure if I want to support that exact use-case, because it means testing all patterns against both regex and regex-lite, and I don't really want to have introduce any kind of feature combination hell into this library and giving up on the --all-features flag Just Working™️. I know this change doesn't exactly cause that kind of problem, but it's a bit too close to that for comfort in my opinion.

That being said, I would be open to changing the Pattern trait similar to how you've done here, but without the direct support of regex-lite. Users wishing to change the regex engine used would have to resort to a newtype.

Here's my very rough idea of how this could work:

  • Remove the feature gate on garde::rules::pattern
  • Matcher trait implemented for regex::Regex (gated behind regex feature)
    trait Matcher {
      fn is_match(v: &str) -> bool;
    }
  • Change Pattern to accept a Matcher:
    trait Pattern {
      fn validate_pattern<M: Matcher>(&self, m: M) -> bool;
    }

If you wanted to use regex-lite with #[garde(pattern(...))] (specifically the expr variant, not the string literal one), you'd implement Matcher on a newtype over regex_lite::Regex.

The original motivation for this change is to allow using regex_lite
instead of regex crate.
@jirutka
Copy link
Contributor Author

jirutka commented Aug 1, 2023

Done, but I’ll recheck it tomorrow.

@jirutka jirutka force-pushed the regex-trait branch 2 times, most recently from 1ac626a to fb12167 Compare August 1, 2023 10:45
@jirutka
Copy link
Contributor Author

jirutka commented Aug 1, 2023

I’ve updated the documentation; it should be ready now.

Just note that the new validate_pattern signature is a better solution per se, but unlike adding a generic parameter with a default value to the trait, this change breaks backward compatibility.

Copy link
Owner

@jprochazk jprochazk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jprochazk jprochazk merged commit 2790b60 into jprochazk:main Aug 1, 2023
@jprochazk
Copy link
Owner

jprochazk commented Aug 1, 2023

Published in v0.14.0

@jirutka jirutka deleted the regex-trait branch August 1, 2023 15:51
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