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

Fix convex function ignoring given properties #2765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kareemjeiroudi
Copy link

@kareemjeiroudi kareemjeiroudi commented Dec 10, 2024

Background

Using the convex function to combine a feature collection returns a newly constructed polygon with missing properties. This crucial if the caller of function attempts to pass additional properties the end polygon.

PR includes a test to test this scenario and improved typing

Resolve #2762

  • Is this a bug fix, new functionality, or a breaking change?
    This change fixes a bug where the convex function ignores given properties. I came across this problem, as I was working on a project for one of our clients
  • Have read and followed the steps for preparing a pull request.

@kareemjeiroudi kareemjeiroudi changed the title Fix convex function ignoring given properties (#2762) Fix convex function ignoring given properties Dec 10, 2024
Copy link
Member

@smallsaucepan smallsaucepan left a comment

Choose a reason for hiding this comment

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

Thanks for this @kareemjeiroudi. Just a request for a minor change to test.ts and we should be good to go!

@@ -32,3 +33,17 @@ test("turf-convex -- empty", (t) => {
t.deepEqual(convex(featureCollection([])), null, "corner case: null hull");
t.end();
});

test("turf-convex -- properties are returned", (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please make the test name more descriptive? Maybe "properties are transferred to result polygon" instead?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Thanks for the feedback. I'll get back to this as soon as back from holidays.

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.

turf.convex ignores given properties
2 participants