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 Serde #9

Closed
wants to merge 4 commits into from
Closed

Implement Serde #9

wants to merge 4 commits into from

Conversation

Deedasmi
Copy link
Contributor

Apologies for the previous PR, I got over-eager and apparently lost my mind. This just simply uses serde's derive functions which should be a lot lower of a maintenance burden at the cost of some more compile time.

Closes #7.

Uses a feature to enable deriving Serialize and Deserialize
@JelteF
Copy link
Owner

JelteF commented Jan 10, 2020

Those changes look fine, but I'm surprised the default itself is part of the serialisation. Is that really useful? I would think it should serialise as a regular map.

Also, can you make some tests that start from a json string? That way it's much clearer what the serialization results in.

@Deedasmi
Copy link
Contributor Author

That's actually what I did in #8, and it worked fine. But had an issue where you could serialize anything that would go into DefaultHashMap, but could only deserialize data that implemented Default back into DefaultHashMap.

Additionally, I came across a different situation where my default value was user-input based. Instead of having to save both when you need both, just decided to serialize it with the map. Can always drop it if you don't need it.

And yeah, I'll add some more test cases soon.

Also some clippy warnings/2018 edition cleanup
@codecov-io
Copy link

codecov-io commented Jan 10, 2020

Codecov Report

Merging #9 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   95.96%   95.83%   -0.14%     
==========================================
  Files           1        1              
  Lines         124      120       -4     
==========================================
- Hits          119      115       -4     
  Misses          5        5
Impacted Files Coverage Δ
src/lib.rs 95.83% <ø> (-0.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f86de74...58bcb91. Read the comment docs.

@JelteF
Copy link
Owner

JelteF commented Sep 6, 2020

Fixed the merge conflict and squashed the commits. This is now on master.

@JelteF JelteF closed this Sep 6, 2020
@JelteF
Copy link
Owner

JelteF commented Sep 6, 2020

Thanks for your work on this.

@JelteF
Copy link
Owner

JelteF commented Sep 10, 2020

I just released this in 0.5.0

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.

Optional "Serde" feature
3 participants