-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add within support to allow iterating over IpNetworks (cidrs) #50
Conversation
} | ||
|
||
impl<'de, T: Deserialize<'de>, S: AsRef<[u8]>> Iterator for Within<'de, T, S> { | ||
type Item = Result<WithinItem<T>, MaxMindDBError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fairly new to rust (in case it's not obvious) and I'm not finding a clean way to go about returning errors that happen during the walking of nodes. My searching for idiomatic patterns didn't turn anything up, but it's a tough thing to search for.
This results in some pretty ugly code below to map errors in the helper functions into Some(Err(e))
. That seems bad enough, but currently have have things in match
Ok
/Err
conditionals that make it tough to read. Things like .map_err
won't let me wrap with Some
so I don't see a cleaner way. Seems like either a shortcoming of iterators or I'm just going about this all wrong 😁
Input/thoughts/suggestions welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there may be a crate, https://docs.rs/fallible-iterator/0.2.0/fallible_iterator/ that calls out the situation.
Seems like using it might be the cleaner way to go, but will defer to more experienced opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a FallibleIterator
based solution and it cleans up pretty much all of the ugliness in error handling here. It's not perfect since you can't use for x in y
, and have to go with while let Some(x) = y.next()
, but that's a limitation of rust and seems like a reasonable tradeoff for cleaner error handling.
Changes for that can be seen with https://github.com/ross/maxminddb-rust/compare/within-support...ross:within-support-fallibleiterator?expand=1. Thoughts welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it does clean up the code somewhat, I think I'd prefer not having the dependency, especially given that the fallible iterator crate hasn't seen an update in two years and doesn't seem that widely used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it hasn't seen any updates b/c it does what it needs to do. Similar to how I wrote requests-futures 8 years ago and haven't really touched it meaningfully in most of that time.
As for not wanting the dep, I don't have a response to that. The code to implement the non-fallible iterator is ugly as hell, but using it isn't THAT bad. Kind of seems like a shortcoming of Rust itself that iterators can't fail.
I'll stick this back on my TODO list to pick back up and see if I can get it over the line.
I think this is ready for 👀 now. |
Sorry for the delay! I forgot about this PR. I've merged it via the CLI. One notable change that I made is that it now skips aliases of the IPv4 network (besides ::/96). I think this is the behavior that generally makes the most sense. I plan on releasing later today. |
This is akin to golang's
within
support, mostly implemented in https://github.com/oschwald/maxminddb-golang/blob/main/traverse.go, but the implementation itself is done from scratch to better match the style and details of this rust implementation.It's still a work in progress, mostly kicking the tires and iterating towards a working/clean solution. The status is roughly:
_
it.