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

feature: add unit option to nearest-point #2415

Merged
merged 5 commits into from
May 8, 2023

Conversation

mwenko
Copy link
Contributor

@mwenko mwenko commented Mar 17, 2023

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.

Feature description

  • The nearestPoint function does not allow to specify the units option. It uses under the hood the distance function, that accepts such an option. This change adds that option as optional to the nearestPoint() function.
  • This allows to specify the unit you wanna have returned in the result properties.distanceToPoint
  • This saves the conversion step of a caller that needs the value in a different unit than kilometers (which is the default)

Tests

  • A test case was added and succeeds.

@JamesLMilner
Copy link
Collaborator

@mwenko this seems like a nice improvement and think it is a good candidate for merging.

Can I request two small changes:

  • Can you update types.ts to include the units option. So basically this line goes from:
const nearest = nearestPoint(targetPoint, points);

to:

const nearest = nearestPoint(targetPoint, points, { units: "kilometers" });
  • Can you please run npm run docs to update the docs

After this I think I can approve

@mwenko
Copy link
Contributor Author

mwenko commented Mar 20, 2023

@mwenko this seems like a nice improvement and think it is a good candidate for merging.

Can I request two small changes:

  • Can you update types.ts to include the units option. So basically this line goes from:
const nearest = nearestPoint(targetPoint, points);

to:

const nearest = nearestPoint(targetPoint, points, { units: "kilometers" });
  • Can you please run npm run docs to update the docs

After this I think I can approve

@JamesLMilner
Thanks for reviewing!
I updated the types & docs. Let me know if there is anything left.

@rowanwins
Copy link
Member

Looks really good @mwenko - just one final little thing and then we'll merge :)

@mwenko mwenko requested a review from rowanwins May 2, 2023 15:20
@rowanwins rowanwins merged commit 88dfdbb into Turfjs:master May 8, 2023
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.

3 participants