impl serde::Deserialize for Wkt and Geometry (and helper function) #59
Conversation
|
bors try |
tryBuild succeeded: |
src/deserialize.rs
Outdated
| /// | ||
| /// #[derive(serde::Deserialize)] | ||
| /// struct MyType { | ||
| /// #[serde(deserialize_with = "wkt::deserialize::deserialize_wkt_geometry")] |
There was a problem hiding this comment.
maybe we can export the module to only do wkt::deserialize_geometry?
There was a problem hiding this comment.
Yes, I thought about something similar, I'm glad you proposed it. Done in f5e5d35.
src/deserialize.rs
Outdated
| use serde::Deserialize; | ||
| use std::convert::TryInto; | ||
| let geometry: Geometry<f64> = Geometry::deserialize(deserializer)?; | ||
| let geometry: geo_types::Geometry<f64> = geometry.try_into().unwrap(); |
There was a problem hiding this comment.
can't we export an error here?
very minor: wouldn't it be better to use std::convert::TryInto; just before using it?
There was a problem hiding this comment.
I agree we don't want to unwrap here, thanks for noticing @antoine-de
There was a problem hiding this comment.
I completely forgot that .unwrap(), oops. Fixed in d5a8de3. I also reorganized the code a bit with a .and_then(), but please tell if you prefer the previous version, I can definitely change it back.
There was a problem hiding this comment.
I also made the helper function generic over the type T of Geometry<T> because the first version was useful for Geometry<f64> but was useless for Geometry<f32>. See commit be202f4.
There was a problem hiding this comment.
These changes all look good to me, thank you!
|
bors try |
tryBuild failed: |
| /// | ||
| /// #[derive(serde::Deserialize)] | ||
| /// struct MyType { | ||
| /// #[serde(deserialize_with = "wkt::deserialize_geometry")] |
There was a problem hiding this comment.
oh right, there is a special dance to do when a doc-test requires a feature flag.
We have a similar example here in proj which requires the optional geo-types feature:
https://github.com/georust/proj/blob/master/src/lib.rs#L175
There was a problem hiding this comment.
Actually, forget that. This whole module is only included conditionally with the serde feature. I'm not immediately sure what to make of the CI failure. I can look at it further next week.
src/lib.rs
Outdated
| extern crate serde; | ||
| #[cfg(feature = "serde")] | ||
| pub mod deserialize; | ||
| #[cfg(feature = "serde")] |
There was a problem hiding this comment.
ah! it's failing because we need both serde and geo-types for this method.
There was a problem hiding this comment.
Ah right, I failed this one. Nice CI setup to catch that by the way. Fixed in fe5e129.
Cargo.toml
Outdated
|
|
||
| [features] | ||
| default = ["geo-types"] | ||
| default = ["geo-types", "serde"] |
There was a problem hiding this comment.
From what I've seen in the ecosystem, Serde support is usually opt-in and not included as a default feature. If I had to guess, I'd say more often then not, users won't need Serde support when using this crate. Would love to hear from others
There was a problem hiding this comment.
Yeah, I think you're right. It should probably not be included in default.
There was a problem hiding this comment.
Oops, this was definitely not intended as I used this trick only locally to facilitate the development. Good catch, thank you 🙏 I amended the last commit.
There was a problem hiding this comment.
bors r+
Ok, I think this has gotten enough feedback. Thanks @woshilapin!
|
Build succeeded: |
CHANGES.mdif knowledge of this change could be valuable to users.