Iter empty db #37
Iter empty db #37
Conversation
|
Thanks @danburkert, your last change looks good to me. I do think it'd make sense to return Result instead of panicking on Err for all of the Cursor::iter*() methods (and, more generally, all public methods of this library), so consumers have better control over handling of error conditions. Nevertheless, that's an orthogonal change that can be considered separately. And in the meantime it makes sense for Cursor::iter_dup_of() to return the same type as the other methods. |
I agree 100% that |
|
(and obviously the issues you pointed out in #32 can cause everything to go off the rails, but that needs to be handled by tweaking the lifetime rules to cause problematic uses of the iterators to fail to compile) |
I'm also not aware of any other ways in which these methods can fail. My statement was just a general one. I'm glad to hear you agree! Would you be amenable to a pull request that made these methods return |
No, because I'm not aware of any situations in which they can fail. There are |
Hmm, I suppose I'm thinking of the errors that we don't expect, and those that don't represent bugs in this library but rather issues encountered by the underlying LMDB C library. The comment on mdb_cursor_get() in lmdb.h only lists MDB_NOTFOUND (which lmdb-rs now handles) and EINVAL (which would suggest a bug in lmdb-rs). But it describes those as merely "some possible errors," suggesting that they're a non-exhaustive list. And mdb_cursor_get() can call mdb_node_read(), which can call mdb_page_get(), which can return MDB_PAGE_NOTFOUND, which would then propagate back to the return value of mdb_cursor_get(). I'm not an expert on LMDB (nor lmdb-rs!), and I don't know if that would actually happen during lmdb-rs use cases. Perhaps that code is never reached. Nevertheless, in general, as a consumer of a library that wraps another library, I would expect to be notified about unexpected error conditions from the underlying library, so I can decide how to respond (f.e. by deleting and recreating a database). And Result seems like the best way to receive such notifications, as it is easier to handle than a panic. |
|
That's a good point, but if those errors are possible and we want to handle them, then not only do |
danburkert
force-pushed
the
iter-empty-db
branch
from
5254516
to
8b5c023
Compare
It previously called unwrap() which panics if a key isn't found
This commit changes the API of Cursor::iter_dup_of, and is thus a breaking change.
danburkert
force-pushed
the
iter-empty-db
branch
from
8b5c023
to
ea2267c
Compare
|
Going to merge this for now, since it fixes some bugs and at least doesn't make the API worse. @mykmelez do you mind creating an issue with your thoughts/suggestions around improving the API in the longer term? |
Sounds like a plan. Will do! |
|
NB: I submitted #42 with my thoughts around improving the API. |
This is a rollup of #7, #28, and #29, as well as a follow up commit of my own. This should fix the empty database panic issue. Thanks to @tarcieri @marshallpierce and @mykmelez for helping with this!