Skip to content
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

Implement optional Serde 1.0 support #28

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

KamilaBorowska
Copy link
Contributor

Note that this is not ready for merging, as it doesn't run Travis tests. I'm not sure what should be testing strategy?

@KamilaBorowska KamilaBorowska mentioned this pull request May 20, 2017
21 tasks
@bluss
Copy link
Member

bluss commented May 24, 2017

One can use a private crate in the repo with extra tests if one wants to split out those tests and their dependencies.

Thanks for working on this.

I kind of want to say that we need to support both kinds of serialization with serde (the kind that gets context from the stream [JSON] and the non-self-describing kind [bincode]). Unfortunately I'm not 100% sure how to ensure compat with both.

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented May 27, 2017

This I'm fairly sure should support both JSON and bincode. All the serialization method does is telling that this is a hash map, and its size (for formats that require size in advance, like bincode). The deserialization method is really simple, it just deserializes a map (while it tells a format that it must be a map) with size that may or may not be specified by a format (JSON doesn't specify map size, bincode does). As long format supports serializing/deserializing hash maps, it will support serializing OrderMap too.

The test function tests if this can be successfully serialized and deserialized - because the types are well known, as long serializer and deserializer support formats that are used (and both JSON and bincode do), it should work.

The thing about tests is that they require serde feature to be enabled to run. I suppose it can be simply enabled for tests? But then lack of serde is not tested.

@LegNeato
Copy link

LegNeato commented Aug 28, 2017

Any chance of getting this merged soon?

@bluss
Copy link
Member

bluss commented Aug 29, 2017

Sure. I'm sorry that it fell of my to do list. Friendly pings are good.

@bluss bluss merged commit 5916167 into indexmap-rs:master Aug 29, 2017
@bluss
Copy link
Member

bluss commented Aug 29, 2017

I'm fixing up .travis.yml

@bluss
Copy link
Member

bluss commented Aug 29, 2017

In 0.2.11

@LegNeato
Copy link

Wow, thanks for the quick turnaround on the merging and release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants