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

pointsWithinPolygon: add MultiPoint support #2137

Merged
merged 13 commits into from
Jul 6, 2021

Conversation

twelch
Copy link
Collaborator

@twelch twelch commented Jun 29, 2021

This PR adds support for MultiPoints to pointsWithinPolygon per issue #2099.

This implements Option 3 discussed, for each MultiPoint passed, returns a MultiPoint with just the point coordinates that fall within a Polygon.

  • 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.

@rowanwins
Copy link
Member

From my initial pass through this all looks good I think @twelch !
I'll just leave @mfedderly to do a final check on some of the ts stuff as that's not really my expertise, but it's got my approval.

Copy link
Collaborator

@mfedderly mfedderly left a comment

Choose a reason for hiding this comment

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

This seems great!

Can you update the @returns annotation to just clarify for JS consumers looking at the docs that the return type mirrors what they passed in?

Also what happens to the typescript types if you pass in a FeatureCollection<Point | Multipoint> for points? Does it allow it and correctly infer FeatureCollection<Point | Multipoint> for the output? Maybe add a types.ts test for that.

* @param {Feature|FeatureCollection<Point>} points Points as input search
* @param {FeatureCollection|Geometry|Feature<Polygon|MultiPolygon>} polygons Points must be within these (Multi)Polygon(s)
* @returns {FeatureCollection<Point>} points that land within at least one polygon
* @param {Feature|FeatureCollection<Point|MultiPoint>} points (Multi)Point(s) as input search
Copy link
Collaborator

Choose a reason for hiding this comment

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

The typescript version of this would be Feature<Point|MultiPoint> | FeatureCollection<Point|Multipoint> but maybe this is fine for JSDoc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, as a TS user myself, your suggestion is more correct but looking at the docs for different functions I sensed there was an established convention already. I don't mind changing them, I would just change the params too to match. Since each type becomes a link in the docs, maybe it was done for simplicity.

image

packages/turf-points-within-polygon/index.js Outdated Show resolved Hide resolved
@twelch
Copy link
Collaborator Author

twelch commented Jul 6, 2021

@mfedderly good suggestions. I have updated the docs and added a mixed Point|Multipoint use case to confirm the returned type is as expected. I hadn't really thought of the user being able to pass in a feature collection containing both types but it is supported so I added a unit test for that as well.

Copy link
Collaborator

@mfedderly mfedderly left a comment

Choose a reason for hiding this comment

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

Thanks! Good point about the jsdoc convention being different than the typescript version. Happy to stick with what we've got already.

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.

None yet

3 participants