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

InvalidGeometryConversion should have "expected" vs. "found" types. #201

Closed michaelkirk closed 2 years ago

michaelkirk commented 2 years ago

Currently when trying to convert from a geojson geometry to an incompatible geo-type (e.g. a Point to a LineString) we use this error:

#[error("Encountered a mismatch when converting to a Geo type: `{0}`")]
InvalidGeometryConversion(geojson::Geometry::Value),

But instead, I think it'd be more helpful as

#[error("Expected geometry type: `{expected_type}`, but found: `{found_type}`")]
InvalidGeometryConversion { expected_type: &'static str, found_type: &'static str }
metasim commented 2 years ago

I think this is a good idea. The former limits control over resource usage just to print an error. Could blow up log messages, etc.

metasim commented 2 years ago

Another options would be to still allow the incompatible geometry to remain as-is so it's available to error handlers, but change the error reporting message to not Display the value.

urschrei commented 2 years ago

In the absence of data to the contrary, I don't think we have to be maximally flexible in our error handling when it comes to conversions (see #176, #197) and I'd be fine with just changing it rather than retaining it without Display.