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 flattenEach/flattenReduce to @turf/meta. #712

Merged
merged 5 commits into from
May 4, 2017
Merged

Add flattenEach/flattenReduce to @turf/meta. #712

merged 5 commits into from
May 4, 2017

Conversation

dpmcmlxxvi
Copy link
Collaborator

This addresses issue #692. There are a few design choices made that deserve scrutiny:

  • @turf/helpers is added as a dependency to @turf/meta. Needed to create features as geometries get flattened.
  • The geomEach callback function was changed to add an additional third argument that passes the properties of the source feature from which the geometry came. Wasn't sure if adding this to the argument list was a good idea since a geometry doesn't have a property per the GeoJSON spec but since geomEach is essentially a flattening function the source will usually be a feature. It's backward compatible as the last argument and existing users can just ignore it.
  • The flattenEach and flattenReduce callback function returns an index argument (increases for each geometry) and an additional subIndex argument that increases for each sub-geometry if the geometry being flattened was a multi-geometry (MultiPoint, MultiLineString, MultiPolygon). For example, for a MultiPoint with, say, 3 points each point will have the same index but their subIndex will be 0, 1, and 2. I'm open to making this a single growing index but I thought this would be helpful if the caller wanted to index back into some property for multi-geometries.

Finally, does Turf have a unit test coverage functionality somewhere? I didn't see any and it would be good to know if the tests are hitting all the right code.

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.

@DenisCarriere
Copy link
Member

DenisCarriere commented May 4, 2017

@turf/helpers is added as a dependency

Don't think this would be necessary, we can include a basic feature() method internally to @turf/meta. No dependencies for this @turf/meta would be ideal.

The geomEach callback function was changed to add an additional third argument that passes the properties of the source feature

👍 That works for me

returns an index argument (increases for each geometry) and an additional subIndex argument that increases for each sub-geometry

I like it! 👍 A growing index could get messy if one needs to find a single part of a MultiFeature.

Few things I did in the recent commits:

  • Converted tests to ES6
  • Dropped @turf/helpers by including a private feature() method (I don't think the helpers validation is necessary in this scenario).
  • Added flattenEach & flattenReduce to typescript definition
  • Updated/Refactored entire typescript definition
  • Add Benchmark results

@dpmcmlxxvi
Copy link
Collaborator Author

OK, will keep in mind newer conventions like ES6 when making any other edits.

@DenisCarriere
Copy link
Member

@dpmcmlxxvi To clarify, ES6 would only apply for the testing (since it's being executed with NodeJS), the index.js syntax would still remain as ES5.

@DenisCarriere
Copy link
Member

@dpmcmlxxvi BTW great job on the PR 🥇 👍

Finally, does Turf have a unit test coverage functionality somewhere? I didn't see any and it would be good to know if the tests are hitting all the right code.

100% agreed, we should have coverage functionality in Turf. I believe I've got a pretty good minimalistic solution for this, i'll send over a new PR for this since we might want to apply this solution to every module.

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