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

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

Closed
michaelkirk opened this issue Aug 19, 2022 · 3 comments · Fixed by #203
Closed

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

michaelkirk opened this issue Aug 19, 2022 · 3 comments · Fixed by #203

Comments

@michaelkirk
Copy link
Member

michaelkirk commented Aug 19, 2022

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 }
  • avoids a very long error message in the case of printing out a large geometry
  • it's a more useful error message in my opinion
  • no longer requires ownership of the geometry value, paving the way for borrowed geometry conversions
@metasim
Copy link
Contributor

metasim commented Aug 19, 2022

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

metasim commented Aug 19, 2022

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
Copy link
Member

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.

bors bot added a commit that referenced this issue Aug 22, 2022
202: Added borrowed value conversion traits for geo_types. r=michaelkirk,urschrei a=metasim

- [X] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [X] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Reworked current implementation to use borrowed values, and created macro to generate owned value variants that delegate to the borrowed value ones.

Note: Until #201 is addressed, invalid geometry conversions will be cloned during error reporting.

Co-authored-by: Simeon H.K. Fitch <fitch@astraea.io>
@bors bors bot closed this as completed in d58a05f Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants