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 FromIterator impl for FeatureCollection #171

Merged
merged 4 commits into from
Aug 21, 2021

Conversation

rmanoka
Copy link
Contributor

@rmanoka rmanoka commented Aug 18, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Implementation Checklist

To summarize, this is the impl. checklist I'm suggesting:

  1. FromIterator for FeatureCollection
  2. From<&'a T> for Feature where From<&'a T> for Value, T: BoundingRect
  3. Ensure correct bbox calculations in 1 and 2 above

@michaelkirk @urschrei I realize we can't use BoundingRect as we only depend on geo-types. Now, I'm a bit hesitant to add impl from geo-types -> Feature given it's just a conversion to Value, then to Feature. The users could do this themselves by converting to Value then using .into() method.

The PR currently just adds FromIterator impl. Should add some tests for this; have an examples collections with bbox to try out with? We could also improve the docs for Value; currently it is hidden deep, and doesn't explain usage.

@michaelkirk
Copy link
Member

I realize we can't use BoundingRect as we only depend on geo-types

Oh right... shoot.

The PR currently just adds FromIterator impl.

The FromIterator implementation looks great to me.

have an examples collections with bbox to try out with

I don't have anything handy.

@rmanoka rmanoka marked this pull request as ready for review August 19, 2021 17:00
@rmanoka
Copy link
Contributor Author

rmanoka commented Aug 19, 2021

Added some minimal test and minor updates to the docs. The main page doc already quite nicely explains how to convert from geo-types, so I just added an example in the FeatureCollection page.

Addresses #170 to some extent

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

All my feedback here should be considered optional - feel free to merge when you're ready.

src/feature_collection.rs Outdated Show resolved Hide resolved
src/feature_collection.rs Outdated Show resolved Hide resolved
@rmanoka
Copy link
Contributor Author

rmanoka commented Aug 19, 2021

I'll leave this open for a day or two if @urschrei would like to add anything. Otherwise, will merge and close #170 .

@urschrei
Copy link
Member

Very happy for this to merge!

@rmanoka
Copy link
Contributor Author

rmanoka commented Aug 21, 2021

bors r=michaelkirk

@bors
Copy link
Contributor

bors bot commented Aug 21, 2021

🔒 Permission denied

Existing reviewers: click here to make rmanoka a reviewer

@lnicola
Copy link
Member

lnicola commented Aug 21, 2021

bors r=michaelkirk

@bors
Copy link
Contributor

bors bot commented Aug 21, 2021

🔒 Permission denied

Existing reviewers: click here to make lnicola a reviewer

@michaelkirk
Copy link
Member

bors r+

(I've also added @rmanoka and @lnicola to be bors reviewers)

@bors
Copy link
Contributor

bors bot commented Aug 21, 2021

Build succeeded:

@bors bors bot merged commit c853106 into master Aug 21, 2021
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.

4 participants