Implement range_de for SnapshotMap #497
Conversation
There was a problem hiding this comment.
Took a quick look and it seems correct to me.
Maybe @maurolacy can give some feedback as you work on porting this to the IndexedMap types. (And I guess we would want such range_de on the indexes??? but that is definitely a separate PR. glad to have a few smaller PRs that can get reviewed and merged quickly.)
| .range_de(&store, None, None, Order::Ascending) | ||
| .collect(); | ||
| let all = all.unwrap(); | ||
| assert_eq!(2, all.len()); |
There was a problem hiding this comment.
Better to remove this.
The next check also asserts this, but gives more info on failure.
There was a problem hiding this comment.
Fair. We do this in other places I think.
There was a problem hiding this comment.
Yes, we started removing some of those in recent PR reviews, which is why I point this out.
Nice to read other PR reviews also to see discussions on APIs and best practices... I think that is where the code style really gets hashed out
| .collect(); | ||
| let all = all.unwrap(); | ||
| assert_eq!(2, all.len()); | ||
| assert_eq!(all, vec![(b"C".to_vec(), 13), (b"D".to_vec(), 22)]); |
There was a problem hiding this comment.
Nice demo showing the deserialisation.
One thing I'm wondering is if we have a desired representation when serializing IndexedMap type stuff, or if it's just a matter of it being consistent with how it's deserialized. |
Deals with #461, but doesn't close it