Improved ergonomics with ToGeoJson and TryFromGeoJson traits

[michaelkirk]

If you've been following the recent changes I made in the WKT crate (georust/wkt#89, georust/wkt#95), I'm interested in doing a similar thing here in the geojson crate: avoid exposing the end user to the intermediate GeoJson struct, and instead let them go directly from their type (e.g. geo-types) to the serialized strings (and back again). This isn't a performance change - it's only a way to expose the user to shorter code with less concepts.

Specifically, I'd like to add two new traits (one for serialization, the other for deserialization), and conform all the geo-types.

For the deserialization, something like:

pub trait TryFromGeoJson<T>: Sized {
    type Error;

    fn try_from_geojson_str(geojson_str: &str) -> Result<Self, Self::Error>;
    fn try_from_geojson_reader(geojson_reader: impl std::io::Read) -> Result<Self, Self::Error>;
}

Which would be used like (equivalent usage with the current API in red):

- use geojson::GeoJson;
- use std::convert::TryInto;
- use std::str::FromStr;
+ use geojson::TryFromGeoJson;

let geojson_str = r#"
{
 "type": "Feature",
 "properties": {},
 "geometry": {
   "type": "Point",
   "coordinates": [
     -0.13583511114120483,
     51.5218870403801
   ]
 }
}
"#;
- let geojson = GeoJson::from_str(geojson_str).unwrap();
- // Turn the GeoJSON string into a geo_types Geometry
- let geom: geo_types::Geometry<f64> = geojson.try_into().unwrap();
- let point = geo_types::Point<f64>::try_from(geom).unwrap();
+ let point = geo_types::Point<f64>::try_from_geojson_str(geojson_str).unwrap();

And for serialization: ToGeoJson is more complicated because of the geometry/feature/feature collection distinction. This is my best guess as to what might be good. I'm interested in feedback if you have any.

trait ToGeoJson {
    fn geojson_geometry(&self) -> geojson::Geometry<T>;
    fn geojson_feature(&self) -> geojson::Feature<T>;
    fn geojson_feature_collection(&self) -> geojson::FeatureCollection<T>;

    fn geojson_geometry_string(&self) -> String;
    fn geojson_feature_string(&self) -> String;
    fn geojson_feature_collection_string(&self) -> String;

    fn write_geojson_geometry(&self, mut writer: impl std::io::Write) -> std::io::Result<()>;
    fn write_geojson_feature(&self, mut writer: impl std::io::Write) -> std::io::Result<()>;
    fn write_geojson_feature_collection(&self, mut writer: impl std::io::Write) -> std::io::Result<()>;
}

Note that in the case of the geo-types integration, no properties would be written. If the user wanted to serialize properties, I guess they'd call geojson_feature() then mutate it. Or they could continue to do whatever they're doing now, without using these API's.

- use geojson::GeoJson;
+ use geojson::ToGeoJson;

  let point = geo_types::Point::new(1.0, 2.0);
- let geojson = GeoJson::from(&point);
- let string = geojson.to_string();
+ let string = point.geojson_geometry_string();
  assert_eq!(string, r#"{"coordinates":[1.0,2.0],"type":"Point"}"#);

The "before" implementation here is actually already pretty concise, but the new code removes the concept of instantiating the intermediate GeoJson type, and also I think it's less surprising that it's returning a geojson geometry string (as opposed to a possibly a feature string).

[michaelkirk]

Thinking about this a bit more - it seems like a common use case would be to build a feature by adding properties to a geo-types geometry.

I think a better approach might be:

trait ToGeoJsonGeometry {

    // Implementor must construct a geojson::Geometry - we might consider a blanket
    // implementation for all `G: Into<geojson::Geometry>`
    fn geojson_geometry(&self) -> geojson::Geometry<T>;

    // the rest of the methods can have a default impl
    fn geojson_geometry_string(&self) -> String {
        self.geojson_geometry().to_string()
    }

    fn geojson_feature_with_props(&self, props: Option<JsonObject>) -> geojson::Feature<T> {
        let mut feature = geojson::Feature::from(self.geojson_geometry());
        feature.props = props
        return feature
    }
 
    fn geojson_feature_with_props_string(&self, props: Option<GeoJsonProperties>) -> String {
        self. geojson_feature_with_props(props).to_string()
    }
}

// anything that can be converted Into a geojson::Geometry can do all 
// the things in `ToGeoJsonGeomtry`
impl<G: Into<geojson::Geometry>> ToGeoJsonGeometry for G {
   fn geojson_geometry(self) {
       self.into()
   }
}

// If your own type is composed of a geometry and its properties, you might want an impl like this
trait ToGeoJsonFeature {
    fn geojson_feature(&self) -> geojson::Feature<T>;
    fn geojson_feature_string(&self) -> String;
}

// Example 1 -  I have my own type that combines geometry and feature data
// so I impl ToGeoJsonFeature.
struct MyPointOfInterest {
    point: geo_types::Point<f32>
    name: String
}

impl ToGeoJsonFeature for MyType {
    fn geojson_feature(&self) -> geojson::Feature<f32> {
        self.point
            .geojson_feature_with_props(Some(json!({"name": self.name.clone()})))
    }
}

let garys_place = MyPointOfInterest { point: geo_types::point!(x: 1., y: 2.), name: "Gary's house" };
assert_eq!(garys_place.geojson_feature_string(), r#"{ "type": "Feature", "geometry": {"coordinates":[1.0,2.0],"type":"Point"}, "properties": {"name": "Gary's house" }}"#)

// Example 2 - Working with geometry only (no feature data)

let point = geo_types::point!(x: 0, y: 1);

// a bare geometry
assert_eq!(point.geojson_geometry_string(), r#"{"coordinates":[1.0,2.0],"type":"Point"}"#)

// a feature with no properties
assert_eq!(point.geojson_feature_with_properties_string(None), r#"{ "type": "Feature", "geometry": {"coordinates":[1.0,2.0],"type":"Point"}}"#)

// Example 3 - Ad hoc feature properties

assert_eq!(point.geojson_feature_with_properties_string(Some(json!({"foo": 1 }), r#"{ "type": "Feature", "geometry": {"coordinates":[1.0,2.0],"type":"Point"}, "properties": {"foo": 1 }}"#)

[themmes]

Indeed the process of adding properties was something I would miss in your original proposal, really like this additional suggestion!

On the function name, why ..with_properties though? If properties is an optional parameter that would be clear right? Not sure, maybe I'm foregoing some Rust best practices..

[michaelkirk]

You're right to bring it up - it feels redundant and verbose.

My thinking was that there was value in having different names for methods across traits:

ToGeoJsonFeature::geojson_feature(&self)
ToGeoJsonGeometry::geojson_feature_with_props(&self, Some(props))

Granted it would work (in that it'd compile) to have them share the same name:

ToGeoJsonFeature::geojson_feature(&self)
ToGeoJsonGeometry::geojson_feature(&self, Some(props))

But the ambiguity could produce some error messages that might be confusing in practice - especially if a type implements both traits:

use geojson::{ToGeoJsonFeature, ToGeoJsonGeometry};
// I think this would be ambiguous and error
foo.geojson_feature()
foo.geojson_feature(props)

// Instead, the user might have to do something like this:
<foo as ToGeoJsonFeature>.geojson_feature()
<foo as ToGeoJsonGeometry>.geojson_feature(props)

I haven't prototyped any of this outside of GH comments yet - maybe it won't be so bad.

[michaelkirk]

I've taken a different approach to the ergonomics problem in #199, based on serde.

I'm curious for any feedback!

(/cc @themmes)