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

include original id in line-slice #2583

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrewharvey
Copy link
Contributor

@andrewharvey andrewharvey commented Feb 28, 2024

The rational here is that slicing a line currently retains the Feature properties, however it drops the Feature id, which may be unexpected for the user.

I'm thinking that we should minimise the side affects of this method, and therefore return the exact same input feature as the output, except with the line geometry sliced.

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

@mfedderly
Copy link
Collaborator

Are you still interested in getting this merged? I'm not sure I understand what the use case would be for this. Can you write up more details about why this has to be implemented inside the method instead of adding the id property to the returned feature?

Thanks!

@andrewharvey
Copy link
Contributor Author

Are you still interested in getting this merged? I'm not sure I understand what the use case would be for this. Can you write up more details about why this has to be implemented inside the method instead of adding the id property to the returned feature?

I wasn't sure if this was a good change, so left it as a draft. At the moment the sliced line retains the original lines properties but not the id and I got caught out by that change.

The argument for retaining the id is slicing a line returns an exact clone of the original line but with a mutated geometry only.

The argument for dropping the id is that the sliced line is no longer the same feature, and shouldn't have the same id.

I think either way it's going to depend on the downstream use to determine if they want to retain the ID, allocate a new one or drop the ID, so regardless we should improve the documentation to make it clear what the user will get back.

I'm slightly leaning in favour of the first option, to retain the id so the side affects of this method are reduced, and it's clear the method is only doing one thing, slicing the geometry.

I'll update the change description.

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.

2 participants