Implement geo_traits for geojson types.
#245
Conversation
I wouldn't necessarily expect the traits to be implemented on |
| @@ -12,11 +12,14 @@ edition = "2018" | |||
|
|
|||
| [features] | |||
| default = ["geo-types"] | |||
| geo-traits = ["dep:geo-traits", "dep:bytemuck"] | |||
There was a problem hiding this comment.
How do you feel about making this a default dependency? I wouldn't imagine it would add too much to compilation time?
There was a problem hiding this comment.
Fine by me! As long as we keep it optional
| match self { | ||
| crate::GeoJson::Feature(f) => f.as_type(), | ||
| crate::GeoJson::FeatureCollection(_fc) => { | ||
| unimplemented!("TODO") |
There was a problem hiding this comment.
TODO here needs to be completed
There was a problem hiding this comment.
I assume a FeatureCollection, interpreted as a geometry-trait, would map to a GeometryCollection?
There was a problem hiding this comment.
Yep exactly. FeatureCollection.as_type is already implemented to return a GeometryCollection. We just need to add that detail here in GeoJson::FeatureCollection.
| } | ||
|
|
||
| fn dim(&self) -> Dimensions { | ||
| self.coord(0).unwrap().dim() // TODO: is this okay? |
There was a problem hiding this comment.
TODO to remove / resolve
There was a problem hiding this comment.
An empty linestring is a normal occurrence, so I don't think we should be unwrap here.
My guess would be something like Dimensions::Unknown(0) or just Dimensions::Xy since it's usually correct.
There was a problem hiding this comment.
Perhaps we should document in geo-traits the expected values of Dimensions::Unknown and whether 0 should be allowed or not.
There was a problem hiding this comment.
We face a similar issue in WKT: georust/wkt#125
WKT seemingly has a solution, but it doesn't really apply to geojson, because the dimensions of geojson are only ever implicit.
There was a problem hiding this comment.
I'd say it's not really an issue in WKT because the WKT string does explicitly say what dimension it is. Here it truly is implicit.
| match self { | ||
| crate::GeoJson::Feature(f) => f.as_type(), | ||
| crate::GeoJson::FeatureCollection(_fc) => { | ||
| unimplemented!("TODO") |
There was a problem hiding this comment.
I assume a FeatureCollection, interpreted as a geometry-trait, would map to a GeometryCollection?
| } | ||
|
|
||
| fn dim(&self) -> Dimensions { | ||
| self.coord(0).unwrap().dim() // TODO: is this okay? |
There was a problem hiding this comment.
An empty linestring is a normal occurrence, so I don't think we should be unwrap here.
My guess would be something like Dimensions::Unknown(0) or just Dimensions::Xy since it's usually correct.
| Self: 'b; | ||
|
|
||
| fn dim(&self) -> Dimensions { | ||
| self.geometry(0).unwrap().dim() |
There was a problem hiding this comment.
Same, we should not crash on a valid geometry like GEOMETRYCOLLECTION EMPTY
| } | ||
|
|
||
| fn dim(&self) -> Dimensions { | ||
| self.line_string(0).unwrap().dim() |
| type GeometryType<'b> = &'b crate::Feature; | ||
|
|
||
| fn dim(&self) -> Dimensions { | ||
| self.features.first().unwrap().dim() |
There was a problem hiding this comment.
just marking all these so they don't get missed.
CHANGES.mdif knowledge of this change could be valuable to users.