pointsWithinPolygon: add MultiPoint support#2137
Conversation
|
From my initial pass through this all looks good I think @twelch ! |
mfedderly
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
The typescript version of this would be Feature<Point|MultiPoint> | FeatureCollection<Point|Multipoint> but maybe this is fine for JSDoc?
There was a problem hiding this comment.
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.
|
@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. |
mfedderly
left a comment
There was a problem hiding this comment.
Thanks! Good point about the jsdoc convention being different than the typescript version. Happy to stick with what we've got already.
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.
npm testat the sub modules where changes have occurred.npm run lintto ensure code style at the turf module level.