Implement regex matching for request path segments #46
Conversation
Adds anchored (^..$) regex matching to the request path segment handling and drops the `unimplemented!()` macro in favor of a concrete implementation. This also adds an `OrderedRegex` newtype as the exist Regex didn't implement the traits required for PartialEq, Eq, PartialOrd, and Ord. See the following Github issues for reasons why Regex doesn't natively implement this: rust-lang/regex#313 rust-lang/regex#364 This (hopefully) addresses and closes #10.
|
I noticed that code coverage decreased likely as a result of adding a type with some manual trait implementations. I can write tests for that type to hopefully restore coverage, but the implementations are trivial. I also can write some docs for the type if they're so requested, but it's not intended to be public to the greater crate. Let me know what you think of the code and if it could eventually be merged. |
There was a problem hiding this comment.
Thanks @ELD and sorry for the delay. This has been sitting with me to finish reviewing.
It works well, and I've only noted a minor improvement that could be made.
src/router/tree/node.rs
Outdated
| fn partial_cmp(&self, other: &OrderedRegex) -> Option<Ordering> { | ||
| return if self.0.as_str() > other.0.as_str() { | ||
| Some(Ordering::Greater) | ||
| } else if self.0.as_str() < other.0.as_str() { |
There was a problem hiding this comment.
Unless I'm mistaken, this could be simplified to:
fn partial_cmp(&self, other: &OrderedRegex) -> Option<Ordering> {
Some(self.0.as_str().cmp(other.0.as_str()))
}The Ord implementation could also be similarly changed to avoid the .unwrap() (though it's just a style change - it won't be possible to trigger a panic there since str has total ordering.)
There was a problem hiding this comment.
Yes, both can be simplified to that. It totally escaped my mind at the time. Thanks for catching that :)
|
Just doing some final checks on this, I've also noted that the |
|
@smangelsdorf I went ahead and added the doc comment for the struct and associated function. I also changed the PartialOrd and Ord implementations in line with your comments. You were right that it could be implemented that way. It escaped my mind at the time I wrote the code but your solution made much more sense than mine. Is it fine to keep them as two separate commits or would you like it to be squashed into one? |
|
Thanks for this @ELD. Great to land this feature! Separate commits are fine, thanks. |
|
@smangelsdorf No problem! I'm happy to contribute to Gotham! |
Adds anchored (^..$) regex matching to the request path segment handling
and drops the
unimplemented!()macro in favor of a concreteimplementation.
This also adds an
OrderedRegexnewtype as the exist Regex didn'timplement the traits required for PartialEq, Eq, PartialOrd, and Ord.
See the following Github issues for reasons why Regex doesn't natively
implement this:
rust-lang/regex#313
rust-lang/regex#364
This (hopefully) addresses and closes #10.
Only concerns with this pull request are the uses of
unwrapwhen constructing the regular expression. Not sure how we want to handle errors for that.