-
Notifications
You must be signed in to change notification settings - Fork 25
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
Implement geo-traits for reading from WKT #123
base: main
Are you sure you want to change the base?
Conversation
type GeometryType<'a> = &'a Wkt<T> where Self: 'a; | ||
|
||
fn dim(&self) -> geo_traits::Dimensions { | ||
if self.0.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be parsed from the input?
GEOMETRYCOLLECTION ZM EMPTY
That might be a larger change though - requiring explicit tracking of the dimensionality of the geometry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't recall that empty geometries could also have a dimension. Does simple features allow that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm not sure. I just assumed that this was valid:
GEOMETRYCOLLECTION Z(POLYGON Z EMPTY, POINT Z(1 2 3))
Maybe it's not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that yes, long-term we should figure out a way to maintain the dimension for empty WKT geometries.
Co-authored-by: Michael Kirk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to make a PR into geo-traits with the unchecked/or_panic variants so we can stop having the same conversation in every geo-traits repo 🤣
type RingType<'a> = &'a LineString<T> where Self: 'a; | ||
|
||
fn dim(&self) -> geo_traits::Dimensions { | ||
// TODO: infer dimension from empty WKT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make an issue describing the work to be done here and any context that we might not remember in a hundred years when someone actually does the work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made #125
(other than the |
Let's release geo-traits-0.2.0 first so we can avoid one sem-ver break. |
CHANGES.md
if knowledge of this change could be valuable to users.Ref georust/geo#1157