-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add type Bbox and trait BoundingBox #41
Conversation
where T: Float | ||
{ | ||
/// | ||
/// Return the BoundingBox for a MultiLineString |
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.
Bad comment
/// let bbox = linestring.bbox().unwrap(); | ||
/// | ||
/// println!("Bbox top left coordinates: {}, {}", bbox.xmin, bbox.ymax); | ||
/// println!("Bbox bottom right coordinates: {}, {}", bbox.xmax, bbox.ymin); |
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.
assert
is generally used in docstring to reflect the expected behavior and because the code is tested by cargo test.
…of a vector of Point<T>
OK for me. I'll merge it by squashing tomorrow if there is no objection. Great work! Thanks for your contribution! |
Oups, forgot to squash. Sorry. |
41: update geo-types to 0.3 r=frewsxcv a=antoine-de Co-authored-by: antoine-de <antoine.desbordes@gmail.com>
I'm not sure that it is right to add the BoundingBox at the rust-geo project, since the boundingbox is not a geometry, but on the other hand it seems a sort of characteristic of the geometry.
What do you think?
Concerning the implementation I don't like too much how the trait BoundingBox is implemented (see: src/algorithm/boundingbox.rs) for MultiLineString and MultiPolygon, because the code is duplicated. I would like to move the code into a private function and call this function from the method, but I was not able to say at the function to take a parameter that can accept only MultiLineString or MultiPolygon. Any ideas on how I can implement this?
Do you see other improvements?