jonathanGB / Unbounded-Interval-Tree

Interval Tree In Rust
https://crates.io/crates/unbounded-interval-tree
MIT License
16 stars 3 forks source link

Add optional `serde` feature for `serde` support. #5

Closed jthulhu closed 2 years ago

jthulhu commented 2 years ago

This commit add a feature "serde" (disabled by default) which pulls in the optional dependency serde and implements Serialize and Deserialize for IntervalTree.

jonathanGB commented 2 years ago

Thanks @TheBlackBeans! Could you please add tests that validate that serialization/deserialization work? I would expect the following cases to be tested:

jthulhu commented 2 years ago

@jonathanGB done. I also added deserialization-only tests, both for empty and non-empty trees, and for leaf and internal nodes.

jthulhu commented 2 years ago

Ops I didn't read the condition height ≥ 2, I'll patch that ASAP.

jthulhu commented 2 years ago

Alright now it should fit your requirements.

jthulhu commented 2 years ago

There we go.

jonathanGB commented 2 years ago

Thanks!

jonathanGB commented 2 years ago

FYI I noticed that we forgot to add the serde feature in the Cargo.toml, thus users of the interval tree can't serialize/deserialize. It worked in the tests because the dependency is not optional in the dev-dependencies.

This is fixed in https://github.com/jonathanGB/Unbounded-Interval-Tree/commit/3a8146dd710bae9fff6ddecac4d0dff93e1e5afb, and the new version is published on crates.io under version 1.1.2: https://crates.io/crates/unbounded-interval-tree/1.1.2.

jthulhu commented 2 years ago

@jonathanGB you don't need to specify a feature in this case. If a dependency is optional, it will automatically create a feature with the same name of the dependency, which will only enable that dependency. This is why in the code #[cfg(feature="serde")] works without having to declare that feature. See the reference of this in the Cargo Book.

I added it as a plain dependency in dev mode simply because it's a pain to launch tests with --features serde or similar stuff, and because my editor reports to me that some pieces of code are disabled if it has a #[cfg(feature="...")] and that the feature is not enabled by default.

jthulhu commented 2 years ago

Also, I noticed that in the latest version of this package, you have moved UnboundedIntervalTree inside the interval_tree module, which is a breaking change, because now you have to use unbounded_interval_tree::interval_tree::IntervalTree instead of unbounded_interval_tree::IntervalTree, so maybe a major version bump would be necessary? Besides that, it's perfect, thanks for the merging the pull!