-
Notifications
You must be signed in to change notification settings - Fork 355
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
Prefixer sub prefix #215
Prefixer sub prefix #215
Conversation
7b4883b
to
4f97caa
Compare
Elide explicit parameter types lifetimes
4f97caa
to
7d1ea98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks
@@ -38,8 +40,9 @@ impl<'a> PrimaryKey<'a> for &'a [u8] { | |||
// Provide a string version of this to raw encode strings | |||
impl<'a> PrimaryKey<'a> for &'a str { | |||
type Prefix = (); | |||
type SubPrefix = (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple. Just add the 2-step variant rather than some generic N-step construction. Should be plenty for any case I can think of.
|
||
fn key<'b>(&'b self) -> Vec<&'b [u8]> { | ||
fn key(&self) -> Vec<&[u8]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And all that was unneeded?
Looks like a better clippy/linter got this working well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something the idea built-in linter recommends. These are called "elided lifetimes", and the linter's recommendation is to remove them.
all, | ||
vec![ | ||
( | ||
[to_length_prefixed(b"\x09"), b"recipient".to_vec()].concat(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the big-endian encoding of U8key manually...
This is fine for now. It would be interesting to see how to auto-parse these keys, but the caller should know how in the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It could be handled automatically by the deserialization function (returning a vec! of (properly deserialized) tuples). But, for that we need to know if the sub-prefix is composed by one or more keys... if all the keys where length-prefixed that would be straightforward: we just honour the length-prefixed information. But currently, the last key is not length-prefixed. So, this introduces ambiguity.
I think an easy fix would be to make all keys length-prefixed, and for the deserialization fn to handle that directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also consider reducing the max length of each length-prefixed key to one byte (255) (which is more than enough for a prefix), to save some bytes / processing.
Implemented first variant for #214.
Update: This turned out to be simpler, as the same
prefix()
method can be used for both, prefixes and sub-prefixes, after signalling which one through its related type alias / helper function.Only missing piece is de-serialization of remaining keys, as they are being returned encoded. This can be tricky, as we don't have a regular format for keys, i. e. the first ones are length prefixed, but the last one isn't.
I think we can live with this: it's an issue only for
Map
, and probablyIndexedMap
. ForUniqueIndex
, and upcomingMultiIndex
with sub-prefixes, this is already taken care of by the custom de-serialization function.