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

Serialization #50

Closed
wants to merge 2 commits into from
Closed

Conversation

HenningTimm
Copy link

This PR adds support for serialization with serde. This is required to add serialization support to BitSet in the bit_set crate as mentioned here.

I increased the version number so that bit_set can require version 0.6.0 since the changes to BitSet would not compile without BitVec implementing the Serialize and Deserialize traits.

The test checks if the content of the array stay the same after serializing it deserializing it again.

serde_json = "1.0"

[dependencies]
serde = "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crate is added, but why is it not listed as extern crate? Is it because of the recent changes in the 2018 Edition? If so, I'm not familiar.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly (almost a year ago), serde_derive did require serde to work, but you did not actually need to call extern crate for it.

@pczarn
Copy link
Contributor

pczarn commented Mar 11, 2019

Please add this support behind an optional feature flag. An example of how to do this exactly:

https://github.com/chronotope/chrono/blob/master/src/lib.rs#L38-L39
https://github.com/chronotope/chrono/blob/master/Cargo.toml#L35

Except that serde_derive should be a regular dependency, not a dev dependency.

@HenningTimm
Copy link
Author

Sorry, this got buried in other GitHub messages.

I did not touch this code in a long time. @mre picked up the code in #57 which is much more recent so it might be better to close this PR in favor of Matthias'.

@pczarn
Copy link
Contributor

pczarn commented May 16, 2019

OK, closing in favor of the more recent PR

@pczarn pczarn closed this May 16, 2019
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.

2 participants