Make Pair more extensible #1101
Conversation
| @@ -2,7 +2,8 @@ use crate::errors::StdError; | |||
| use std::convert::TryFrom; | |||
|
|
|||
| /// A Key-Value pair, returned from our iterators | |||
| pub type Pair<V = Vec<u8>> = (Vec<u8>, V); | |||
| /// (since it is less common to use, and never used with default V, we place K second) | |||
There was a problem hiding this comment.
Do we really want this? It makes sense, but it feels unnatural.
I did something similar with Prefix, but in that case V was mandatory, and putting K second was the way to make it optional.
Then I realized that if we make V optional (i.e. with a default), the changes to support the "natural" order are relatively straightforward.
There was a problem hiding this comment.
I'll publish that into my draft PR.
In my opinion it would be better to try and support the K, V order. I don't know about the changes needed in Pair's current usage, though.
There was a problem hiding this comment.
We have Pair used in many places throughout the code (especially in storage-plus)
I assume this will be the most common usage.
I was trying to avoid breakage. You can make the version with breakage and see which we like more.
There was a problem hiding this comment.
I'd also favour a version where the generic arguments are in the same order as the two fields. If this was a struct with two fields, the order would not matter that much. But here we have unnamed positional fields.
I see Ethan's point as well. We try to do a lot of different things with one symbol. In cosmwasm_std we don't need the generic arguments at all because all is binary. Then we build higher and higher and using the same type. I'm not surprised this ends up non-ideal. But at the end it is probably more beginner friendly to only have one type.
By the way, since this is a generic pair, we probably should rename the arguments to A and B for .0 and .1.
There was a problem hiding this comment.
By the way, since this is a generic pair, we probably should rename the arguments to A and B for .0 and .1.
This will be super confusing. When I get it I usually write something like |(k, v)| Ok(String::from_utf8(&k), from_slice(&v))
Having A and B would make it more confusing.
This was previously named KV before being renamed to Pair and it is not intended to be generic, but return data from an Storage Iterator.
|
Discussed in #1106 Not going to change it, but use a new type in storage-plus where we want a generic K. This type works well for this one use case |
Closes #1100