Skip to content
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

Merged
merged 21 commits into from
Jul 21, 2016
Merged

Add type Bbox and trait BoundingBox #41

merged 21 commits into from
Jul 21, 2016

Conversation

zarch
Copy link
Contributor

@zarch zarch commented Jul 17, 2016

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?

where T: Float
{
///
/// Return the BoundingBox for a MultiLineString
Copy link
Contributor

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);
Copy link
Contributor

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.

@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 20, 2016

OK for me. I'll merge it by squashing tomorrow if there is no objection.

Great work! Thanks for your contribution!

@TeXitoi TeXitoi merged commit a05b31a into georust:master Jul 21, 2016
@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 21, 2016

Oups, forgot to squash. Sorry.

@zarch zarch deleted the bbox branch July 22, 2016 04:40
nyurik pushed a commit to nyurik/geo that referenced this pull request Mar 19, 2022
41: update geo-types to 0.3 r=frewsxcv a=antoine-de



Co-authored-by: antoine-de <antoine.desbordes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants