georust / gpx

Rust read/write support for GPS Exchange Format (GPX)
https://crates.io/crates/gpx
MIT License
98 stars 44 forks source link

Implementing serde (De)Serialize for GPX structs #62

Closed mkroehnert closed 2 years ago

mkroehnert commented 2 years ago

This PR resolves #59, by implementing optional support for (de-)serializing the GPX structs via serde.

I named the feature flag serde-serialize, same as in the wasm-bindgen crate. Using serde for the feature flag name is not yet possible, since it would require the currently unstable cargo namespaced features implementation. See also the related cargo tracking issue.

It was asked in #59, whether it would be possible to use GeoJSON, but for my usecase, it would be overkill to deal with additional data structure conversions. I also checked the Rust API guidelines, which mention that it would be good, if structs implemented serde::{Serialize, Deserialize}.

If the changes are okay and can/should be merged, I'll update the branch with a changelog entry.

lnicola commented 2 years ago

I named the feature flag serde-serialize, same as in the wasm-bindgen crate.

I think other crates call it use-serde: https://github.com/georust/geo/blob/master/geo/Cargo.toml#L30-L33. serde-serialize is a bit misleading -- before reading the code I thought it only included serialization.

mkroehnert commented 2 years ago

@lnicola thanks for the review. I rename the feature to use-serde and applied your fix.

lnicola commented 2 years ago

And finally, can you add a changelog entry?

I think this is fine otherwise, but I'll give it a day or so in case anyone else wants to take a look.

mkroehnert commented 2 years ago

@lnicola done. Thank you very much for the reviews.

lnicola commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded: