Skip to content
This repository was archived by the owner on Sep 13, 2023. It is now read-only.

return error result from fallible iteration methods #13

Closed
wants to merge 5 commits into from

Conversation

mykmelez
Copy link

@mykmelez mykmelez commented Nov 19, 2018

This branch implements the suggestion in danburkert#42 by returning an error result from fallible iteration methods (Cursor.iter*() and Iterator.next()) when those methods fail. See that issue for more info about the changes.

(I've previously requested integration of these changes upstream in danburkert#44 and would like to move forward with these changes downstream while awaiting that integration.)

match self.get(Some(key.as_ref()), None, ffi::MDB_SET_RANGE) {
Ok(_) | Err(Error::NotFound) => (),
Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error),
Err(error) => return Err(error),
Copy link
Member

Choose a reason for hiding this comment

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

Is this just e => e? Or is there a hidden conversion happening between Result errors?

Copy link
Author

Choose a reason for hiding this comment

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

This is just e => e.

Copy link
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

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

Generally, I'm fine with this approach, since it's both clear and minimizes dependencies. However, I'd like to make sure you're aware of https://github.com/sfackler/rust-fallible-iterator, which brings a little more machinery to the party.


assert_eq!(0, cursor.iter_dup_of(b"foo").count());
assert_eq!(0, cursor.iter_dup_of(b"foo").unwrap().count());
Copy link
Member

Choose a reason for hiding this comment

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

For next time: generally I like .expect("...") in tests, since it can help narrow down what really broke. This is especially useful when you have two .unwrap() invocations on a single line (which you do).

Copy link
Author

Choose a reason for hiding this comment

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

Roger that. In this case, The changes are maintaining consistency with the existing code, which uses unwrap() everywhere.

src/cursor.rs Outdated
@@ -571,10 +584,30 @@ mod test {
let mut i = 0;
let mut count = 0u32;

for (key, data) in cursor.iter().map(|x| x.unwrap()) {
Copy link
Member

Choose a reason for hiding this comment

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

Meh; these little lambda functions are pretty idiomatic.

src/cursor.rs Outdated
@@ -602,10 +602,6 @@ mod test {
let mut i = 0;
let mut count = 0u32;
for result in cursor.iter() {
// let (key, data) = match result {
Copy link
Member

Choose a reason for hiding this comment

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

I did wonder.

@mykmelez
Copy link
Author

mykmelez commented Dec 3, 2018

Generally, I'm fine with this approach, since it's both clear and minimizes dependencies. However, I'd like to make sure you're aware of https://github.com/sfackler/rust-fallible-iterator, which brings a little more machinery to the party.

Thanks, that's good to know. If I understand correctly, fallible_iterator::convert() will convert an Iterator<Item = Result<T, E>> into a FallibleIterator<Item = T, Error = E>, so consumers who want that functionality can wrap the iterators that lmdb-rs returns. So I haven't converted them to FallibleIterator here (but will keep it in mind).

@mykmelez
Copy link
Author

mykmelez commented Dec 3, 2018

Since #14 subsumes this PR (includes these changes), and you seemed optimistic about that one (after some changes) when we discussed it, I'll close this PR in favor of that one.

@mykmelez mykmelez closed this Dec 3, 2018
@mykmelez mykmelez deleted the return-error-result branch February 12, 2019 00:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants