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

deprecate: GeometryCollection::from(single_geom). It doesnt give us much and causes awkwardness downstream. #821

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Apr 27, 2022

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

As with most blanket implementations, the problems are little convoluted to explain.

Maybe easier to explain is that, as is, this implementation is only marginally useful. Having to write GeometryCollection::new(vec![point]) isn't much more arduous than writing GeometryCollection::from(point), and arguably clearer.

But the actual trouble I hit was while working on the WKT crate (georust/wkt#95, see the second commit in particular).

Here's a simplified example of the kind of thing I was trying to do but can't (without jumping through some hoops):

// Any type that can be built from our serialized wkt representation can impl this trait.
trait FromSerialized<T> {
    fn from_wkt(wkt: &str) -> Result<Self>;
}

// A blanket implementation exists for all the things that can be (fallibly)
// built from a Geometry - i.e. all the inner variants (Point, Line, etc., but NOT a GeometryCollection)
impl<T: CoordNum, IG> FromSerialized<T> for IG
where
IG: TryFrom<Geometry<T>> + Into<Geometry<T>> + Default
{
    fn from_wkt(wkt: &str) -> Self {
        let g: Geometry<T> = parse_geometry_from_wkt(wkt);
        let specific = Self::try_from(g).map_err(|_| "e.g. Got LineString, expected Point").unwrap();
        specific
    }
}

fn parse_geometry_from_wkt<T: CoordNum>(wkt: &str) -> Geometry<T> {
    todo!("pretend this does something interesting, like parse WKT")
}

mod tests {
    use crate::{Point, LineString, GeometryCollection};
    use super::{FromWkt};

    #[test]
    fn example() {
        // All geometry variants *except* GeometryCollections can utilize the blanket implementation above.
        let point = Point::from_serialized("POINT(1 2)").unwrap();
        let line_string = LineString::from_wkt("LINESTRING(1 1, 2 2)").unwrap();

        // !!! But GeometryCollection::from_serialized will not compile...
        // ... because it has no `impl TryFrom<Geometry>`, because that impl would conflict (be ambiguous) with the existing
        // (proposed deprecation) `impl TryFrom<Into<Geometry>> for GeometryCollection`
        let geom_collection: GeometryCollection<f64> = GeometryCollection:: from_wkt("GEOMETRYCOLLECTION(("POINT(1 2)))");
    }
}

I worked around it in the WKT crate, but it was awkward. And I don't think many people would be sad if the conflicting implementation went away.

If people are commonly creating GeometryCollections from a single geometry, I think something like GeometryCollection::from_single(point) (or whatever) would be less obtrusive.

@michaelkirk michaelkirk force-pushed the mkirk/deprecate-geom-collection-from-single-geom branch from 00d4d01 to b4511aa Compare April 27, 2022 23:16
@michaelkirk michaelkirk force-pushed the mkirk/deprecate-geom-collection-from-single-geom branch from b4511aa to d6d386e Compare May 11, 2022 22:22
As with most blanket implementations, the problems are little convoluted to
explain, but I hit this while working on the WKT crate.

Here's an example of the kind of thing I was trying to do but can't (without jumping through some hoops):
```
trait FromSerialized<T> {
    fn from_serialized(serialized: &str) -> Self;
}

impl<T: CoordNum, IG> FromSerialized<T> for IG
where
IG: TryFrom<Geometry<T>> + Into<Geometry<T>> + Default
{
    fn from_serialized(serialized: &str) -> Self {
        let g: Geometry<T> = parse_geometry(serialized);
        let specific = Self::try_from(g).map_err(|_| "Got Foo, expected Bar").unwrap();
        specific
    }
}

fn parse_geometry<T: CoordNum>(_serialized: &str) -> Geometry<T> {
    todo!("pretend this does something interesting, like parsing WKT")
}

mod tests {
    use crate::{Point, GeometryCollection};
    use super::{FromSerialized};

    #[test]
    fn example() {
        // All geometries *except* GeometryCollections will compile
        let point: Point<f64> = Point::from_serialized("asdf");

        // But GeometryCollection cannot compile!
        // It has no `impl TryFrom<Geometry>`, because such an impl would conflict with the existing
        // (now deprecated) `impl TryFrom<Into<Geometry>> for GeometryCollection`
        let geom_collection: GeometryCollection<f64> = GeometryCollection::from_serialized("asdf");
    }
}
```

I think it'll be possible to work around it in the WKT crate, but it'll
be a little awkward, and I don't think we're getting much mileage out of
this impl.

If people are commonly creating GeometryCollections from a single geometry, I
think something like `GeometryCollection::from_one(point)` (or whatever) would
be less obtrusive.
@michaelkirk michaelkirk force-pushed the mkirk/deprecate-geom-collection-from-single-geom branch from d6d386e to 94e8fca Compare May 11, 2022 22:25
@michaelkirk
Copy link
Member Author

rebased to fix conflicts in the change log

michaelkirk added a commit to georust/geojson that referenced this pull request Jun 1, 2022
WIP: Note the crates.io patch to use an unpublished geo. See
georust/geo#821
@michaelkirk
Copy link
Member Author

bors r=frewsxcv

@bors
Copy link
Contributor

bors bot commented Jun 2, 2022

Build succeeded:

@bors bors bot merged commit 5f21059 into main Jun 2, 2022
@bors bors bot deleted the mkirk/deprecate-geom-collection-from-single-geom branch June 2, 2022 19:17
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 this pull request may close these issues.

2 participants