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

Geojson ids option #220

Merged
merged 8 commits into from
May 6, 2024
Merged

Geojson ids option #220

merged 8 commits into from
May 6, 2024

Conversation

Mr-Ixolate
Copy link
Contributor

PR for #219

Change makes it so that geojson.FeatureCollections with Features that have ids will have those ids passed through to the output of Topology.
A new option ignore_index that allows that behavior to be reverted to what it was before also added. Left off by default

Added three tests.

  1. Checking index is passed through extract
  2. Checking duplicate index raises an error
  3. Checking that ignore_index=True uses the original "feature_0", "feature_0" index

Two tests that counted the number of options were modified to increase the expected count by one.

If "id" doesn't exist then the original feature_{index} applies.
If they were then the data dict will have overwritten the previous feature rather than adding a new feature and its length will be shorter than the original length of the features.

Not sure about raising error or just warning.
added description of param to Topology docstring
ignore_index being true will go to the default index likewie if the feature has no id it will use the default index
Copy link
Owner

@mattijn mattijn left a comment

Choose a reason for hiding this comment

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

Thank you @Mr-Ixolate! This PR is in excellent condition, awesome!
Thank you for adding the extra tests, all remaining tests are happy too. Merging!

(FYI, I will release a new version within a week or two)

@mattijn mattijn merged commit 025c0e6 into mattijn:main May 6, 2024
6 checks passed
Copy link

@Hlmt7 Hlmt7 left a comment

Choose a reason for hiding this comment

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

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