simpler key

[enex]

In my opinion the way the key is implemented is not very userfriendly, becaus the user has to define a new type to use it with the database. Instead just using &[u8] or the following would make it much simpler.

pub trait Key<'a>: From<&'a[u8]> + AsRef<&'a[u8]>{}

Another way would be to implement Key for &[u8].

[skade]

I don't agree that &[u8] would make things much simpler. Considering how important it is in leveldb to implement key ordering, having the key as separate type makes sense in my opinion. It might be a bump to get started with, but is better in the long run.

See e.g. https://github.com/skade/drossel-journal/blob/master/src/lib.rs, which implements a journal for queue.

Thanks for bringing up the From + AsRef-suggestion though, both traits didn't exist when I implemented the key, so I will probably go with that.

[icorderi]

A built-in implementation for &[u8], String, u32 and u64 would go a long way on user friendliness for the 95% of cases. Currently the library only comes with i32.

[skade]

I'm working on this, though I must add that this probably means a bit more effort then expected. Using & [u8] internally means, that the key type would gain a lifetime through From<& [u8]> :(. I'd like to avoid that.

https://github.com/skade/key/blob/master/tests/key.rs#L35

[skade]

After a quite disheartening discussion about this here, I'll stick with my own conversion trait and implement a few more standard things.

http://users.rust-lang.org/t/searching-for-a-nice-to-and-from-binary-slice-abstraction/1092/8

[skade]

After thinking about this for a while, noting things down: Key should remain a trait and implementations should implement Borrow< > and Into< > for it.

[skade]

I decided to go a different route: The key implementation stays similar and requires conversion two and from a pointer and a length. For convenience, this comes with a default implementation for types that are Sized + Copy. See https://github.com/skade/db-key/tree/sized-key for details.

[aij]

Um, doesn't that implementation just flat out break type safety?

Yes, yes it does:
http://is.gd/h5PxTf

(The above example dereferences an arbitrary int (42) in safe code.)

IMHO, the main problem with the old API is just that it leaves no room for
error handling, which the new API doesn't solve.

On Sat, Aug 29, 2015 at 8:20 AM, Florian Gilcher [email protected]
wrote:

I decided to go a different route: The key implementation stays similar
and requires conversion two and from a pointer and a length. For
convenience, this comes with a default implementation for types that are Sized

• Copy. See https://github.com/skade/db-key/tree/sized-key for details.

—
Reply to this email directly or view it on GitHub
#6 (comment).

[skade]

Neither of the approaches suggested here does error handling, especially concerning when using From and friends. It could be solved in this one.

Considering safety concerns, you are right. Although more because of wrongfully making the trait unsafe instead of the conversion methods, this would fit the intention better: http://is.gd/ZV7q0l

(same boom, but because of misusing two unsafe methods)

[aij]

Either way, I'm not sure why you'd want to make db-key unsafe...

The default implementations Ignacio is asking for could be done with the
old API.

On Sat, Aug 29, 2015 at 3:16 PM, Florian Gilcher [email protected]
wrote:

Neither of the approaches suggested here does error handling (which could
be solved in this one).

Considering safety concerns, you are right. Although more because of
wrongfully making the train unsafe instead of the conversion methods, this
would fit the intention better: http://is.gd/ZV7q0l

(same boom, but because of misusing two unsafe methods)

—
Reply to this email directly or view it on GitHub
#6 (comment).

[skade]

After thinking about it for a weekend, I think the important part here is assuming that the underlying library handles raw pointers.

I'm currently contemplating going the way Path goes: be a type built over &[u8], with conversion traits to and from being implemented for most types.

[tmccombs]

What is the current status of this? It seems like Vec and String at the very least should implement the Key trait. But since an implementation isn't provided in the same crate as Key, I have to use a wrapper type. This is rather inconvenient.

It also really seems like I should be able to use a &[u8] without copying into owned memory.

I'm happy to help with implementing something, but I want to implement the right thing :)

[tmccombs]

With the current API I can't even figure out how to use a wrapper around &[u8] where lifetimes work out. I think part of the solution would be to use &Key (in which case the type of Key could be [u8]). The problem is with iteration.

One way that might work could be something like this:

trait Key {
  type Iterated: Borrow<Self>;
  fn to_u8(&self) -> &[u8];
  fn from_u8(bytes: &[u8]) -> Iterated;
}

With an implementation for [u8] that looked like

impl Key for [u8] {
  type Iterated = &[u8];
  fn to_u8(&self) -> &[u8] { self }
  fn from_u8(bytes: &[u8]) -> Iterated { bytes }
}

And then get and put would take a &Key for the key argument and the key returned by iterators would be of type Key::Iterated

[skade]

This sounds like a good way. I must admit that a lot of the design around keys was me learning Rust, so I should change that soon (or accept PRs? :) )

[oconnor663]

It looks like this has been changed, but the documentation hasn't been updated to reflect it: https://skade.github.io/leveldb/db_key/trait.Key.html

I just tried to create a hello-world-style example to print all the keys in a database, and I wasn't able to figure out how to do it. Could you update the README with an example where the keys are strings?

[kaimast]

I think you should have a similar API to the RocksDB crate, where instead of a dedicated Key trait the get and put functions are templates that take a From<&[u8]> and As<&[u8]> respectively.

Would a pull request for something like that be accepted or is this crate in full maintenance mode?

[skade]

@kaimast as long as there's a reasonable backwards-compatibility story, I'm okay with such a change. Not sure how many projects actually express a dependency to the key trait,

My first pass would be to actually remove the dependency from db-key, to maybe open it up for a better design.