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 the bugs of generating topojson with complicated geojson #154

Merged
merged 3 commits into from
Apr 7, 2022

Conversation

natsuapo
Copy link
Contributor

@natsuapo natsuapo commented Apr 1, 2022

I did the test again with proper path and debug the code.

This time there are three fails in all test:
Two fails are related to test_extract in the assertion of intermediate dict structure. I think these fails are inevitable since I changed the process structure of feature and geometry collections.

One fail is related to test_topology_to_geojson_nested_geometrycollection in test_topology. This is also an assertion fail as in my process, I changed the geojson structure of nested geometrycollection, so it cannot be reverted to the same geojson after it is converted to topojson. But as far as I read the generated geojson, I think they can refer to the original geojson with the same geometry.

I am sorry that I cannot solve the remaining fails since they are much related to other data structures. But I think at least this adaption can be utilized for generating topojson from geojson with attributes. I am not sure whether to merge it or not, so I leave the decision to the maintainers.

@mattijn
Copy link
Owner

mattijn commented Apr 1, 2022

Thank you for pushing! Last evening I tried to solve the issue as well and the changes were near identical to yours, but I've to check once again when I find some time.

I'm not really font of the (nested) geometrycollections within the geojson dataformat.

I also still had some failing tests elsewhere, but that might be indeed because of changes regarding improved serialization.

Thanks again!

@mattijn mattijn merged commit 6c140a8 into mattijn:master Apr 7, 2022
@mattijn mattijn mentioned this pull request Apr 7, 2022
@mattijn
Copy link
Owner

mattijn commented Apr 7, 2022

Merged and update the remaining tests. Thanks for the PR👍

@mattijn mattijn mentioned this pull request Apr 7, 2022
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.

None yet

2 participants