-
Notifications
You must be signed in to change notification settings - Fork 198
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
Poof of concept: geo_traits::Coord
#1169
base: main
Are you sure you want to change the base?
Conversation
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 be potentially interested in merging a Coord trait (though I have a bikeshed preference for calling it Point
, see: #1157 (comment)
One thought is we could incubate it in geo
, which has breaking releases relatively often, and promote it to geo_types
once we feel it's stable.
I did have a couple of concerns with this PR about the custom debug impl and explicit type annotations. In the actual code in this PR it's obviously NBD, but I'm wondering if they are indicative of changes that will break for downstream users and if there's anything we could do to prevent them.
@@ -58,10 +60,13 @@ fn get_min_max<T: PartialOrd>(p: T, min: T, max: T) -> (T, T) { | |||
} | |||
} | |||
|
|||
pub fn line_segment_distance<T, C>(point: C, start: C, end: C) -> T | |||
pub fn line_segment_distance<T, C>( | |||
point: impl Into<C>, |
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.
Do we need the Into
? Can't we call the methods directly on our C
?
@@ -32,7 +32,7 @@ mod test { | |||
let point = Point::new(1.0, 2.0); | |||
let point_geometry = Geometry::from(point); | |||
|
|||
let rect = Rect::new(Point::new(1.0, 2.0), Point::new(3.0, 4.0)); | |||
let rect = Rect::<Coord>::new(Point::new(1.0, 2.0), Point::new(3.0, 4.0)); |
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.
Hm... was this not compiling without this change?
pub struct PointsIter<'a, C: geo_traits::Coord + 'a>(::core::slice::Iter<'a, C>); | ||
|
||
impl<'a, C: fmt::Debug + geo_traits::Coord + 'a> fmt::Debug for PointsIter<'a, C> { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { |
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.
Do you know why we have to roll our own Debug here?
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.
Fixed in 559dc72
CHANGES.md
if knowledge of this change could be valuable to users.