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 topojson object path #1665

Merged
merged 3 commits into from
Nov 27, 2022

Conversation

Conengmo
Copy link
Member

@Conengmo Conengmo commented Nov 26, 2022

Closes #828

Fix an issue with Topojson object paths. With the current code each item in the path should be a valid Javascript identifier, but users may not know this.

I looked into JS identifier validation, but I don't think that's the way to go:

  • Topojson doesn't demand the objects have valid JS identifiers as names.
  • There are many ways in which a JS identifier can be invalid, so it's hard to test.
  • Users may be unable to change their Topojson data easily.

Instead I opted to convert the path to an array notation: obj1.obj2 -> ["obj1"]["obj2"]. After we escape double quotes any string should work. With this, object paths that include hyphens work.

Alternatively, only look for hyphens and raise an error if we see one. I'd rather fix the complete problem instead of just one specific case. But if this change is too complex, that might be a small step to take instead.

@Conengmo
Copy link
Member Author

@ocefpaf or @martinfleis, would one of you mind reviewing this change?

Copy link
Collaborator

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

This looks good.

Alternatively, only look for hyphens and raise an error if we see one. I'd rather fix the complete problem instead of just one specific case. But if this change is too complex, that might be a small step to take instead.

It doesn't seem to be an overly complex solution. I am fine with it and agree that it is better to fix the whole issue where it starts rather than a specific subset of it.

@Conengmo
Copy link
Member Author

Thank you! Appreciate the review!

@Conengmo Conengmo merged commit 762c876 into python-visualization:main Nov 27, 2022
@Conengmo Conengmo deleted the fix-topojson-object-path branch November 27, 2022 15:26
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.

Folium can't parse TopoJSON objects with dashes - in name
2 participants