Skip to content

Publicly export Snapshot (and minor changes) #59

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

robofinch
Copy link
Contributor

Motivation

I'm making a crate (well, collection of crates) for reading and writing MCBE worlds, and wanted to abstract over the implementation of LevelDB with a generic parameter, bounded by a trait specifying the interface. This trait includes a Snapshot associated type, and when I went to implement the trait for DB... well, rusty_leveldb::snapshot::Snapshot isn't publicly exported.

While I was at it, I looked for other visibility issues, and adjusted WriteBatch and DBIterator. The compiler also warned me about an issue in mem_env.rs.

Changes

  • Explicitly use FileExt::unlock in mem_env.rs to avoid a potential conflict with std::fs::File::unlock once the latter becomes stable. (File::unlock is/will be an inherent method, not a method from a trait, so it'd win the conflict.)
  • Publicly reexport Snapshot, accessible as rusty_leveldb::Snapshot.
  • Make the new() constructor of WriteBatch public (it was already publicly reachable anyway, as WriteBatch::default()).
  • While I'm at it, improve the documentation for WriteBatch's encoding format. (I'm pretty sure I got the details right, but you might want to double-check.)
  • Make WriteBatch::clear leave 12 header bytes set to zero, in line with WriteBatch::new, to ensure that the preceding calls to put or delete are cleared, and following puts and deletes can be performed. If this didn't count as a bug fix, it could require a larger version bump if semver were followed more strictly, but previously WriteBatch::clear would prevent following operations on it (except for WriteBatch::set_contents) from working properly; AFAICT there can't be any existing usages of WriteBatch::clear that would break which weren't already broken.
  • Remove one unnecessary call to WriteBatch::clear in db_impl.rs, which would precede a call to WriteBatch::set_contents in a following loop iteration. set_contents already clears the Vec inside WriteBatch, so clear did and does nothing there.
  • Change DBIterator::new to not be visible outside the crate, since one of the arguments to new (MergingIter) isn't publicly exported, and isn't returned from a publicly exported function (at least among what docs.rs can see).

Notes

I don't know why SkipMap is publicly exported. But I don't think anyone outside the crate would be able to use SkipMap for anything. Might as well leave it to avoid the breaking change of removing it, but the next time the major version number is bumped, SkipMap should probably be limited to within the crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant