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

Implementing serde (De)Serialize for GPX structs #62

Merged
merged 3 commits into from Oct 3, 2021
Merged

Implementing serde (De)Serialize for GPX structs #62

merged 3 commits into from Oct 3, 2021

Conversation

mkroehnert
Copy link
Contributor

@mkroehnert mkroehnert commented Oct 3, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

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.

Cargo.toml Outdated Show resolved Hide resolved
@lnicola
Copy link
Member

lnicola commented Oct 3, 2021

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
Copy link
Contributor Author

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

@lnicola
Copy link
Member

lnicola commented Oct 3, 2021

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.

@lnicola lnicola added the hacktoberfest-accepted Hacktoberfest approved label Oct 3, 2021
@mkroehnert
Copy link
Contributor Author

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

Cargo.toml Outdated Show resolved Hide resolved
@lnicola
Copy link
Member

lnicola commented Oct 3, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 3, 2021

Build succeeded:

@bors bors bot merged commit 2c79d11 into georust:master Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementing serde (De)Serialize for GPX structs?
2 participants