georust / geojson

Library for serializing the GeoJSON vector GIS file format
https://crates.io/crates/geojson
Apache License 2.0
276 stars 60 forks source link

Add (de)serialize_optional_geometry helpers. #235

Closed gibbz00 closed 11 months ago

gibbz00 commented 11 months ago

Before

#[derive(Deserialize)]
struct MyStruct {
    count: usize,
    geometry: Option<GeometryWrapper>,
}

#[derive(Deserialize)]
#[serde(transparent)]
struct GeometryWrapper(#[serde(deserialize_with = "deserialize_geometry")] Point<f64>);

After:

#[derive(Deserialize)]
struct MyStruct {
    count: usize,
    #[serde(deserialize_with = "deserialize_optional_geometry")]
    geometry: Option<Point<f64>>,
}

michaelkirk commented 11 months ago

Hmmm... I haven't worked with this code for a bit, but I'm surprised that you'd need a custom deserialization method for this.

You can already do an Option<Geometry> with vanilla serde derive, right? I wonder why that wouldn't work for Point or the other Geometry enums 🤔

michaelkirk commented 11 months ago

I'm sorry, I missed that this was about geo-types. I think the approach makes sense. 👍

gibbz00 commented 11 months ago

Sounds good 😊

michaelkirk commented 11 months ago

Note to self and other maintainers... the merge queue (which we recently adopted) seemed to merge this branch immediately without waiting for tests to complete (though the tests ultimately did pass, so no worries).

I updated a couple settings which I think we were missing, so it should work next time. 🤞

Most importantly the "require status checks" setting:

Screenshot 2023-11-29 at 10 42 02

I also enabled "require PR before merging" like we do in geoerust/geo.:

Screenshot 2023-11-29 at 10 42 05