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.

Sign up for GitHub

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

Specify the details of conversion failures in an exported error enum #614

Merged
merged 2 commits into from
Feb 6, 2021
Merged

Specify the details of conversion failures in an exported error enum #614

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df01c04
deprecate error which is only used by deprecated method
michaelkirk Jan 29, 2021
9c2d7b5
conversion error specifies type
michaelkirk Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
conversion error specifies type 
This also makes public a new geo_types::Error enum.
  • Loading branch information
@michaelkirk
michaelkirk committed Feb 6, 2021
commit 9c2d7b575c071c4c2458f6450af56c089c654afc
2 changes: 2 additions & 0 deletions geo-types/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

* Implement `Default` on `Coordinate` and `Point` structs (defaults to `(x: 0, y: 0)`)
* <https://github.com/georust/geo/pull/616>
* Add specific details about conversion failures in the newly public `geo_types::Error`
* <https://github.com/georust/geo/pull/614>

## 0.7.0

Expand Down
44 changes: 44 additions & 0 deletions geo-types/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use std::fmt;

#[derive(Debug)]
pub enum Error {
MismatchedGeometry {
expected: &'static str,
found: &'static str,
},
}

impl std::error::Error for Error {}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Error::MismatchedGeometry { expected, found } => {
write!(f, "Expected a {}, but found a {}", expected, found)
}
}
}
}

#[cfg(test)]
mod test {
use crate::{Geometry, Point, Rect};
use std::convert::TryFrom;

#[test]
fn error_output() {
let point = Point::new(1.0, 2.0);
let point_geometry = Geometry::from(point);

let rect = Rect::new(Point::new(1.0, 2.0), Point::new(3.0, 4.0));
let rect_geometry = Geometry::from(rect);

Point::try_from(point_geometry).expect("failed to unwrap inner enum Point");

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

@michaelkirk michaelkirk Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Member Author

@michaelkirk michaelkirk Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

);
}
}
137 changes: 49 additions & 88 deletions geo-types/src/geometry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use crate::{
CoordNum, GeometryCollection, Line, LineString, MultiLineString, MultiPoint, MultiPolygon,
Point, Polygon, Rect, Triangle,
CoordNum, Error, GeometryCollection, Line, LineString, MultiLineString, MultiPoint,
MultiPolygon, Point, Polygon, Rect, Triangle,
};
use core::any::type_name;
use std::convert::TryFrom;
use std::error::Error;
use std::fmt;

/// An enum representing any possible geometry type.
///
Expand Down Expand Up @@ -182,94 +181,56 @@ impl<T: CoordNum> Geometry<T> {
}
}

#[derive(Debug)]
pub struct FailedToConvertError;
Copy link
Member Author

@michaelkirk michaelkirk Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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 fmt::Display for FailedToConvertError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "Could not convert from enum member to concrete type")
}
}

impl Error for FailedToConvertError {
fn description(&self) -> &str {
"Could not convert from enum member to concrete type"
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for Point<T> {
type Error = FailedToConvertError;

fn try_from(geom: Geometry<T>) -> Result<Point<T>, Self::Error> {
match geom {
Geometry::Point(p) => Ok(p),
_ => Err(FailedToConvertError),
#[macro_use]
macro_rules! try_from_geometry_impl {
($($type: ident),+) => {
$(
/// Convert a Geometry enum into its inner type.
///
/// Fails if the enum case does not match the type you are trying to convert it to.
impl <T: CoordNum> TryFrom<Geometry<T>> for $type<T> {
type Error = Error;

fn try_from(geom: Geometry<T>) -> Result<Self, Self::Error> {
match geom {
Geometry::$type(g) => Ok(g),
other => Err(Error::MismatchedGeometry {
expected: type_name::<$type<T>>(),
found: inner_type_name(other)
})
}
}
}
)+
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for Line<T> {
type Error = FailedToConvertError;
try_from_geometry_impl!(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Point,
Line,
LineString,
Polygon,
MultiPoint,
MultiLineString,
MultiPolygon,
Rect,
Triangle
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should GeometryCollection be in here? Nvm nvm nvm


fn try_from(geom: Geometry<T>) -> Result<Line<T>, Self::Error> {
match geom {
Geometry::Line(l) => Ok(l),
_ => Err(FailedToConvertError),
}
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for LineString<T> {
type Error = FailedToConvertError;

fn try_from(geom: Geometry<T>) -> Result<LineString<T>, Self::Error> {
match geom {
Geometry::LineString(ls) => Ok(ls),
_ => Err(FailedToConvertError),
}
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for Polygon<T> {
type Error = FailedToConvertError;

fn try_from(geom: Geometry<T>) -> Result<Polygon<T>, Self::Error> {
match geom {
Geometry::Polygon(ls) => Ok(ls),
_ => Err(FailedToConvertError),
}
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for MultiPoint<T> {
type Error = FailedToConvertError;

fn try_from(geom: Geometry<T>) -> Result<MultiPoint<T>, Self::Error> {
match geom {
Geometry::MultiPoint(mp) => Ok(mp),
_ => Err(FailedToConvertError),
}
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for MultiLineString<T> {
type Error = FailedToConvertError;

fn try_from(geom: Geometry<T>) -> Result<MultiLineString<T>, Self::Error> {
match geom {
Geometry::MultiLineString(mls) => Ok(mls),
_ => Err(FailedToConvertError),
}
}
}

impl<T: CoordNum> TryFrom<Geometry<T>> for MultiPolygon<T> {
type Error = FailedToConvertError;

fn try_from(geom: Geometry<T>) -> Result<MultiPolygon<T>, Self::Error> {
match geom {
Geometry::MultiPolygon(mp) => Ok(mp),
_ => Err(FailedToConvertError),
}
fn inner_type_name<T>(geometry: Geometry<T>) -> &'static str
where
T: CoordNum,
{
match geometry {
Geometry::Point(_) => type_name::<Point<T>>(),
Geometry::Line(_) => type_name::<Line<T>>(),
Geometry::LineString(_) => type_name::<LineString<T>>(),
Geometry::Polygon(_) => type_name::<Polygon<T>>(),
Geometry::MultiPoint(_) => type_name::<MultiPoint<T>>(),
Geometry::MultiLineString(_) => type_name::<MultiLineString<T>>(),
Geometry::MultiPolygon(_) => type_name::<MultiPolygon<T>>(),
Geometry::GeometryCollection(_) => type_name::<GeometryCollection<T>>(),
Geometry::Rect(_) => type_name::<Rect<T>>(),
Geometry::Triangle(_) => type_name::<Triangle<T>>(),
}
}
3 changes: 3 additions & 0 deletions geo-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ mod rect;
#[allow(deprecated)]
pub use crate::rect::{InvalidRectCoordinatesError, Rect};

mod error;
pub use error::Error;

#[macro_use]
mod macros;

Expand Down