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

Support GeometryCollection in @turf/line-chunk. #726

Merged
merged 1 commit into from
May 10, 2017
Merged

Support GeometryCollection in @turf/line-chunk. #726

merged 1 commit into from
May 10, 2017

Conversation

dpmcmlxxvi
Copy link
Collaborator

Addresses #710 for line-chunk to add GeometryCollection support. Updates include:

  • Replaces featureEach/flatten with flattenEach.
  • Adds GeometryCollection test fixtures.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.


/**
* Divides a {@link LineString} into chunks of a specified length.
* If the line is shorter than the segment length then the original line is returned.
*
* @name lineChunk
* @param {FeatureCollection|Feature<LineString|MultiLineString>} featureIn the lines to split
* @param {FeatureCollection|Feature|GeometryCollection<LineString|MultiLineString>} featureIn the lines to split
Copy link
Member

@DenisCarriere DenisCarriere May 10, 2017

Choose a reason for hiding this comment

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

For the JSDocs I've been using Geometry to encompass all Geometry Collection & Geometry Objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. A GeometryCollection is a Geometry per the spec. In my head I think of them as different since it's strange to think of a collection of something to also be that something, but it is.

Copy link
Member

Choose a reason for hiding this comment

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

I've never really understood the difference between both of them until you've pointed it out to me. Geometry Collections is hard one to support, however with the latest flattenEach method it's pretty simple now.
+1

feature.geometry.coordinates = feature.geometry.coordinates.reverse();
var lineSegments = sliceLineSegments(feature, segmentLength, units);
lineSegments.forEach(function (segment, index) {
if (debug === true) {
Copy link
Member

Choose a reason for hiding this comment

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

Didn't notice (debug===true) was still here, this section can be removed since it can easily be handled in the test.js.

// Flatten each feature to simple LineString
flattenEach(featureIn, function (feature) {
if (reverse) {
feature.geometry.coordinates = feature.geometry.coordinates.reverse();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would cause the input geojson to be mutated, I'll add a test case for that and see if this method causes input mutation.

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

Looks great! Including flattenEach makes a world of difference!

@DenisCarriere
Copy link
Member

@dpmcmlxxvi Next time can you create your branches in TurfJS, doesn't seem like I can push commits to this PR. I'll merge this PR and add my commits afterwards.

@DenisCarriere DenisCarriere merged commit 46b8502 into Turfjs:master May 10, 2017
DenisCarriere added a commit that referenced this pull request May 10, 2017
- Dropped colorized source code, placed in testing instead.
- Update Typescript definition to support Geometry Objects
- Udpate Benchmark results
- Improve tests to catch Geometry Objects & Input Mutation
- Update authors
- Add reverse to typescript definition
Ref: #726
CC: @dpmcmlxxvi
@dpmcmlxxvi
Copy link
Collaborator Author

Will do.

@DenisCarriere
Copy link
Member

Added to this PR:

  • Dropped colorized source code, placed in testing instead.
  • Update Typescript definition to support Geometry Objects
  • Update Benchmark results
  • Improve tests to catch Geometry Objects & Input Mutation
  • Update authors
  • Add reverse to typescript definition

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