FromWkt trait for reading WKT without exposing the user to the intermediate representation. #95
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelkirk
force-pushed
the
mkirk/from-wkt
branch
from
5d700a0 to
a826970
Compare
2 tasks
urschrei
approved these changes
Apr 28, 2022
…ediate representation. Note that this isn't a performance change. It's about (hopefully) making the library easier to use. This is a corollary to #89, but for reading WKT, rather than writing. As we discussed there, probably there is no reason for the user to care about the Wkt struct. Note that the intermediate representation is still used (for now!), but the user is no longer required to interact with it. The road is open though for having a direct translation from Wkt text to the geo-types (or whatever) represenation (see geozero for inspiration).
michaelkirk
force-pushed
the
mkirk/from-wkt
branch
from
a826970 to
9fecace
Compare
|
bors r=urschrei (rebased) |
|
Build succeeded: |
bors bot
added a commit
to georust/geo
that referenced
this pull request
Jun 2, 2022
821: deprecate: GeometryCollection::from(single_geom). It doesnt give us much and causes awkwardness downstream. r=frewsxcv a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] 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](georust/wkt@9fecace#diff-1f4c8d43a2fd4d510da5fb47f3421e5457d297b4f3cffa665392e79c4774af2cR109). 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. Co-authored-by: Michael Kirk <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGES.mdif knowledge of this change could be valuable to users.Draft because it's based on #94. Please review that first.
Note that this isn't a performance change. It's about (hopefully) making
the library easier to use.
This is a corollary to #89, but for
reading WKT, rather than writing. As we discussed there, probably there
is no reason for the user to care about the Wkt struct.
Note that the intermediate representation is still used (for now!), but
the user is no longer required to interact with it.
The road is open though for having a direct translation from Wkt text to
the geo-types (or whatever) represenation (see geozero for inspiration).
I also added the missing GeometryCollection::from(wkt) implementation to make this work for all geo-types.