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

Adding explode turf method #1007

Merged
merged 1 commit into from
May 2, 2019
Merged

Conversation

langsmith
Copy link

This pr ports the explode method to Turf for Java. This method takes a Feature or FeatureCollection and returns a FeatureCollection of Point objects which represent all of the positions in that original Feature/FeatureCollection parameter.

http://turfjs.org/docs/#explode

https://github.com/Turfjs/turf/blob/master/packages/turf-explode/index.js

explode(FeatureCollection featureCollection) & explode(Feature feature) are the new methods added in the TurfConversion class.

Screen Shot 2019-04-19 at 11 05 16 AM

@langsmith langsmith requested a review from osana April 19, 2019 19:10
@langsmith langsmith self-assigned this Apr 19, 2019
@langsmith langsmith force-pushed the ls-adding-turf-explode-method branch 3 times, most recently from c95c4c1 to 12d807c Compare April 19, 2019 19:17
@langsmith
Copy link
Author

@osana , for some reason, the test wouldn't accept my fixture for Point and Polygon. The GeoJSON is valid if you check it with geojson.io.

That's why I'm building those object in a different way for now:

Is there something I'm doing wrong, which is preventing me from doing Polygon polygon = Polygon.fromJson(loadJsonFixture(TURF_EXPLODE_POLYGON)); ?

Also, https://github.com/mapbox/mapbox-java/pull/1007/files#diff-b6a32aa24e73a2791a14203a75659fce is in this pr because it was invalid GeoJSON. It needed properties. Not sure how the bbox tests ✅ with this error before adding this properties field 🤔

* @since 4.7.0
*/
private static FeatureCollection explode(@NonNull Geometry geometry) {
List<Feature> featuresForFinalFeatureCollection = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is a method:
https://turfjs.org/docs/#coordAll
that is supposed to work for FeatureCollection/Feature and Geometry.
We ported only the Geometry parameter.
What do you think if we port coordAll(FeatureCollection) and coordAll(Feature) and then take advantage of it here.

Copy link
Author

Choose a reason for hiding this comment

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

What do you think if we port coordAll(FeatureCollection) and coordAll(Feature) and then take advantage of it here.

Yea, I think we should do this. I've needed it a bunch during my work on porting more methods to Turf for Java.

Copy link
Author

Choose a reason for hiding this comment

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

Yea, I think we should do this.

#1009

Copy link
Author

Choose a reason for hiding this comment

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

@osana , just refactored and rebased this pr. Added usage of the new coordAll() methods, which were added in #1009.

@langsmith langsmith force-pushed the ls-adding-turf-explode-method branch 2 times, most recently from 7a22877 to 8228322 Compare April 25, 2019 14:30
@osana osana added this to the v4.8.0 milestone May 2, 2019
@langsmith langsmith force-pushed the ls-adding-turf-explode-method branch from 8228322 to eb524ca Compare May 2, 2019 18:28
@langsmith langsmith merged commit 86b0594 into master May 2, 2019
@langsmith langsmith deleted the ls-adding-turf-explode-method branch May 2, 2019 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants