-
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
Fix missing prefix bound types #655
Conversation
@@ -172,11 +172,11 @@ where | |||
K: PrimaryKey<'a>, | |||
I: IndexList<T>, | |||
{ | |||
pub fn sub_prefix(&self, p: K::SubPrefix) -> Prefix<K::SuperSuffix, T> { | |||
pub fn sub_prefix(&self, p: K::SubPrefix) -> Prefix<K::SuperSuffix, T, K::SuperSuffix> { |
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.
Ugh, so what is 3rd type argument in Prefix
again?
pub struct Prefix<K, T, B = Vec<u8>>
where
K: KeyDeserialize,
T: Serialize + DeserializeOwned,
{
storage_prefix: Vec<u8>,
data: PhantomData<(T, B)>,
pk_name: Vec<u8>,
de_fn_kv: DeserializeKvFn<K, T>,
de_fn_v: DeserializeVFn<T>,
}
It's the actual content of prefix, right?
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.
The 3rd type is precisely the new Bound
type annotations. You can compare with Map
to get the idea.
While implementing this, made it work for Map
, and then forgot about these here and elsewhere (MultiIndex
). These bounds have a default of Vec<u8>
, that's why it "worked" (passed existing tests).
Adding explicit tests for these typed bounds would be good I guess, at some point.
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.
It's a little tricky how all this works. I can give a small overview / talk at some point, if there's interest.
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.
Adding explicit tests for these typed bounds would be good I guess, at some point.
I think this is worth an extra issue as follow up.
c295fe7
to
ea6601e
Compare
Fixes a couple of errors with missing bound annotations for
IndexedMap
s andMultiIndex
.