billyrieger / bimap-rs

Generic bijective maps in Rust

Apache License 2.0

129 stars 26 forks source link

Serde Serialize/Deserialize implementations #7

Closed dmarcuse closed 5 years ago

dmarcuse commented 5 years ago

This PR adds implementations of serde::Serialize and serde::Deserialize for BiHashMap and BiBTreeMap, gated behind the serde and std features.

Progress:

[x] Implementations of Serialize and Deserialize
[x] Documentation with examples
[x] Unit tests
[x] Update README/crate documentation

dmarcuse commented 5 years ago

@billyrieger Quick question: When deserializing a map, the code will currently silently overwrite pairs if already present. Should this be changed? I think that this behavior is fine since it should only be possible to have conflicting pairs in the serialized format if using a different serializer anyways.

codecov-io commented 5 years ago

Codecov Report

Merging #7 into master will increase coverage by 0.06%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   99.24%   99.31%   +0.06%     
==========================================
  Files           3        4       +1     
  Lines         532      582      +50     
==========================================
+ Hits          528      578      +50     
  Misses          4        4

Impacted Files	Coverage Δ
src/lib.rs	`100% <ø> (ø)`	:arrow_up:
src/serde.rs	`100% <100%> (ø)`

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 338e315...85d1b5a. Read the comment docs.

dmarcuse commented 5 years ago

The tests are using the Serialize/Deserialize implementations, so I'm not sure why codecov isn't picking it up. Any ideas?

billyrieger commented 5 years ago

@dmarcuse re: silently overwriting on deserialization, I think it's fine, it's the same as the current behavior of FromIterator (which I probably should have documented the behavior of... I'll do that before the next release).

It looks like the coverage report is skipping over serde.rs because it's behind the serde feature flag which isn't enabled by default. Running cargo tarpaulin --all-features locally gives an accurate coverage report. FYI everything is covered except the two expecting methods from the Visitor impls. To fix this you can change the relevant line in .travis.yml from cargo tarpaulin --out Xml to cargo tarpaulin --all-features --out Xml. Not sure if that'll change the Codecov bot but it should at least change the actual coverage when the PR is merged.

I'll look over the PR with a closer eye later today but overall looks great! Thanks especially for the in-depth documentation.

dmarcuse commented 5 years ago

Alright, I'll change the travis config. If there's anything else you think I should change, just let me know!

dmarcuse commented 5 years ago

I just pushed the changes and reformat you suggested - I didn't realize GitHub allowed you to accept suggestions until after committing, whoops

dmarcuse commented 5 years ago

It also seems like updating the Travis configuration fixed the code coverage check, but it's almost 1% lower! (Oh no!)

billyrieger commented 5 years ago

It's all good, I've never used suggestions before this and just wanted to give them a try.

As for the coverage, the missing lines are the expecting methods in the Visitor impls: here and here. I'm working on adding some tests to cover them.

billyrieger commented 5 years ago

Alright, ready to merge :fireworks: thanks again for this PR!

dmarcuse commented 5 years ago

Thank you!