danburkert / lmdb-rs

Safe Rust bindings for LMDB
Apache License 2.0
172 stars 87 forks source link

return Result from fallible iteration methods #42

Open mykmelez opened 6 years ago

mykmelez commented 6 years ago

In #37, I suggested returning a Result instead of panicking when iterator operations fail.

The Cursor::iter*() methods are straightforward to convert, and the ergonomic impact would be minimal. To obtain the equivalent behavior as today (panic on error), a consumer would just need to unwrap the Result:

- cursor.iter().collect::<Vec<_>>()
+ cursor.iter().unwrap().collect::<Vec<_>>()

Alternately, they could handle it via the other Result methods, via a match operator, or (in functions that themselves return a Result) via the ? operator, which is a single character change:

- cursor.iter().collect::<Vec<_>>()
+ cursor.iter()?.collect::<Vec<_>>()

Iter::next() is similarly straightforward to convert, although the ergonomic impact seems more significant. Nevertheless, iterating Result instances appears to be common enough that idioms have emerged to simplify it.

To obtain the equivalent behavior as today, a consumer could unwrap() each Result:

- cursor.iter().collect::<Vec<_>>()
+ cursor.iter().map(Result::unwrap).collect::<Vec<_>>()

However, they could also convert a collection of results—f.e. Vec<Result<T, E>>—into a result containing a collection (or the first error)—i.e. Result<Vec<T, E>> via this compact syntax as a type annotation:

let items: Result<Vec<_>> = cursor.iter().collect();

Or the turbofish equivalent:

cursor.iter().collect::<Result<Vec<_>>>()

The consumer could then return the error in a function that itself returns Result via the ? operator:

- cursor.iter().collect::<Vec<_>>()
+ cursor.iter().collect::<Result<Vec<_>>>()?

Or handle it in another way that is appropriate to its use case.

Also see Karol Kuczmarski's blog post Iteration patterns for Result & Option, which describes other interesting options, such as collecting only Ok results (ignoring errors) and partitioning results into separate collections of Ok and Err results.

for loops over iterators can similarly be converted to obtain the equivalent behavior as today:

- for (key, data) in cursor.iter() {
+ for (key, data) in cursor.iter().map(Result::unwrap) {

To return on error, however, I think a consumer would need to destructure the tuple in the loop body:

- for (key, data) in cursor.iter() {
+ for result in cursor.iter() {
+     let (key, data) = result?;

@danburkert Do these idioms seem ergonomic enough, or are they still too complex for comfort?

mykmelez commented 6 years ago

44 implements the API I've described here. #45 enhances that API to avoid having to return a result from the Cursor::iter*() methods, making them infallible, so the consumer only has to check the result of Iter::next().