-
Notifications
You must be signed in to change notification settings - Fork 199
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
Converter functions for geo-traits to geo-types #1255
base: main
Are you sure you want to change the base?
Conversation
geo-traits/src/to_geo.rs
Outdated
if let Some(coord) = point.coord() { | ||
Point(coord_to_geo(&coord)) | ||
} else { | ||
panic!("converting empty point to geo not implemented") |
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.
If we can panic from user input, it should be documented under a # Panics
heading.
In general I wish we'd think harder about how to avoid panics. Please read my rant if you haven't already: #1244
In this case, how about returning an Option<Point<T>>
. Note it would cascade to the to_geometry function as well.
If you think it's not worth it, then an alternative I'd be willing to move forward with would be to document these panics, and add non-panic
versions that return Option: try_point_to_geo
.
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 agree this panic is unfortunate and not ideal.
In this case, how about returning an
Option<Point<T>>
. Note it would cascade to theto_geometry
function as well.
I'm open to this for to_point
but I think it seems rather bad for to_geometry
.
Perhaps the best way is to have panicking and non-panicking variants? 🤷♂️
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.
Yeah, panicking and try_*
non-packing variants seems like a good middleground.
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 added panicking and non-panicking variants
geo-traits/src/to_geo.rs
Outdated
/// Convert any coordinate to a [`Coord`]. | ||
/// | ||
/// Only the first two dimensions will be kept. | ||
pub fn coord_to_geo<T: CoordNum>(coord: &impl CoordTrait<T = T>) -> Coord<T> { |
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.
Since these functions are exported from the to_geo
mod, should we name it to_coord
instead of coord_to_geo
?
use geo_traits::to_geo;
to_geo::to_coord(...);
to_geo::to_point(...);
etc.
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.
Or should that be: to_geo::from_coord(some_coord_trait)
🤯😱💥
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.
To be clear, I don't really know. I do think your shorter name is better than what's there current and unlikely to be confusing in context (assuming the caller keeps the to_geo
in scope).
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.
Either way. I don't have a preference. Maybe from_coord
is preferable in the hope that some day in the future the traits are stable enough that we could have geo_types::Coord::from_coord
?
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.
You could do that today if you introduced a trait:
in geo-traits crate:
impl FromPointTrait {
fn from_point(point: impl PointTrait) -> Self;
}
impl FromPointTrait on geo::Point {
...
}
And then also in other crates:
impl FromPointTrait for wkt::Point {
...
}
Then you could do things like:
let geo_point = geo::Point::new(1.0, 2.0);
let wkt_point = wkt::Point::from_point(geo_point);
let back_to_geo_point = geo::Point::from_point(wkt_point);
Is that useful? I dunno. For some formats it'd be a nice conversion muxer between formats. Though I don't know that it would work for all formats where there isn't a "standalone" structure for one geometry (e.g. converting something to a single standalone GeoArrow point might be weird, right?)
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.
Rather I could imagine a trait being defined like
I like that. I'm happy with either merging something like ToGeoCoord
or to_coord
as you previously phrased.
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've #1157 (comment) that I don't think the traits should necessarily provide a way to construct new objects. I think that should be implementation defined.
In any case, I'm not advocating for adding a new
method to CoordTrait, like you were arguing against in that comment, but rather to add a new trait designating "This thing can be constructed from a impl geo-traits
"
I think that's basically the From
foil of what you're proposing in To
form with ToGeoCoord
.
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.
Right, that's an important distinction. Especially because the From
could be optionally implemented and isn't a requirement to implement the trait.
Rather I could imagine a trait being defined like
I like that. I'm happy with either merging something like
ToGeoCoord
orto_coord
as you previously phrased.
Happy to do whatever. Any preferences?
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.
Dealers choice.
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 switched to traits. Do you like this impl?
CHANGES.md
if knowledge of this change could be valuable to users.Closes #1253