Specify the details of conversion failures in an exported error enum #614
Conversation
| impl std::error::Error for InvalidRectCoordinatesError {} | ||
|
|
||
| #[allow(deprecated)] |
There was a problem hiding this comment.
This deprecation stuff is sort of orthogonal. We deprecated the only method that produces this error, so we should also deprecate the error itself - in hopes that we can delete them both one day.
| @@ -182,94 +181,56 @@ impl<T: CoordNum> Geometry<T> { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| pub struct FailedToConvertError; | |||
There was a problem hiding this comment.
🚨 I deleted this FailedToConvertError.
My understanding is that this is not a breaking change, because, though it was public, it was never exported - so no external crate could have typed in geo_types::FailedToConvertError ...so non-breaking, right?
| } | ||
| } | ||
|
|
||
| impl<T: CoordNum> TryFrom<Geometry<T>> for Line<T> { | ||
| type Error = FailedToConvertError; | ||
| try_from_geometry_impl!( |
There was a problem hiding this comment.
I'm starting to leverage more macros to DRY up our code. I'm pretty new to using macros, so if this feels sophomoric or something, let me know - I'm not actually that bothered by the mostly copy-pasted implementations that I replaced.
There was a problem hiding this comment.
I am completely clueless when it comes to writing macros but this one is readable and I understand what it's doing, so I think that's a good sign?
| let failure = Point::try_from(rect_geometry).unwrap_err(); | ||
| assert_eq!( | ||
| format!("{}", failure), | ||
| "Expected a geo_types::point::Point<f64>, but found a geo_types::rect::Rect<f64>" |
There was a problem hiding this comment.
here's what the error output actually looks like -
before:
Could not convert from enum member to concrete type
after:
Expected a geo_types::point::Point<f64>, but found a geo_types::rect::Rect<f64>
There was a problem hiding this comment.
An example of where I'd like to take advantage of these more specific error messages, is in code like this georust/wkt#57
let wkt = wkt::Wkt::from_str(include_str!("../resources/test_fixtures/louisiana.wkt")).expect("invalid test data");
let ls = geo::LineString::<f32>::try_from(wkt).expect("unexpected geometry in wkt");
michaelkirk
force-pushed
the
mkirk/pub-error
branch
2 times, most recently
from
42ed0f4 to
b21321c
Compare
michaelkirk
force-pushed
the
mkirk/pub-error
branch
from
b21321c to
5c05bdc
Compare
| MultiPolygon, | ||
| Rect, | ||
| Triangle | ||
| ); |
There was a problem hiding this comment.
Should Nvm nvm nvmGeometryCollection be in here?
| impl std::error::Error for InvalidRectCoordinatesError {} | ||
|
|
||
| #[allow(deprecated)] |
|
bors r+ |
|
Merge conflict. |
This also makes public a new geo_types::Error enum.
michaelkirk
force-pushed
the
mkirk/pub-error
branch
from
5c05bdc to
9c2d7b5
Compare
|
bors retry (rebased and fixed conflict in CHANGES.md) |
|
Build succeeded: |
57: specific errors and `try_into` for inner geo-types r=urschrei a=michaelkirk - [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. --- **DRAFT**: depends on georust/geo#614 being released first. Add TryFrom for converting directly to `geo_types::Geometry` enum members. ``` let ls: geo_types::LineString = geo_types::LineString::try_from(wkt).unwrap(); ``` Previously it was: ``` let geom: geo_types::Geometry = geo_types::Geometry::from(wkt).unwrap(); let ls: geo_types::LineString = geo_types::LineString::try_from(geom).unwrap(); ``` This introduced two new error cases, so I've also introduced `thiserror` which solves #49 FIXES #49 Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>
CHANGES.mdif knowledge of this change could be valuable to users.I came across this while trying to improve ergonomics of the WKT crate, which relies on this conversion logic. (see georust/wkt#57)
I think this is not a breaking change (see comment inline), but would appreciate confirmation. If I'm wrong, and it indeed is a breaking change, I'd prefer to hold off on merging it for now.