Fix TopoJson non-collection geometries - #2250
Conversation
|
This fixes #1816 — TopoJson raised a KeyError('geometries') when the Fix checks Added a parametrized test covering both Polygon and MultiPolygon cases. |
adarshsm
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @KhadeerBasha1232 — the normalize-to-a-list approach is the right shape. A few things I hit while testing it against #1816:
- Type-key robustness:
geometry["type"] == "GeometryCollection"will itself raiseKeyError: 'type'if an object omitstype. Keying on the member you actually need —"geometries" in geometry— sidesteps that and reads as "does this hold sub-geometries, or is it one itself?" - Test could assert styling, not just rendering: since
style_data's job is to attachproperties.style, passing astyle_functionand asserting the style lands would catch a future regression that silently drops styling. - There's a second crash site this doesn't reach.
style_dataruns before children render, so once it's fixed, a single-geometry TopoJSON with a fields-basedGeoJsonPopup/GeoJsonTooltiphits the same assumption again atfeatures.py~L1205 (["geometries"][0]["properties"]). Minimal repro:I have a fix + regression tests for both sites ready — happy to push it as a follow-up PR once this lands, or you're welcome to fold the second-site fix in here. Whatever you and the maintainers prefer.import folium from folium.features import GeoJsonPopup topo = {"type": "Topology", "objects": {"A": {"type": "Polygon", "arcs": [[0]], "properties": {"name": "A"}}}, "arcs": [[[0, 0], [100, 0], [100, 100], [0, 100], [0, 0]]], "transform": {"scale": [1e-4, 1e-4], "translate": [9.47, 46.34]}} m = folium.Map() t = folium.TopoJson(topo, "objects.A") t.add_child(GeoJsonPopup(fields=["name"])) t.add_to(m) m.render() # KeyError: 'geometries'
|
Thanks for the thorough review, @adarshsm — really appreciate you digging into the edge cases and providing the repro script. Pushed a follow-up commit (7157e9b) that addresses all three points: Type-key robustness — switched the check to "geometries" in geometry instead of geometry["type"] == "GeometryCollection", so it no longer breaks on objects missing a type key. Added a regression test (test_topojson_non_collection_geometry_without_type) covering that exact case. All local tests pass (pytest tests/test_features.py) along with pre-commit. Let me know if you'd like anything else adjusted — happy to iterate further. |
|
That's a clean set of changes, @KhadeerBasha1232 — the shared |
Fixes #1816
Problem
When a TopoJSON's
objectsentry had atypeofPolygonorMultiPolygon(instead ofGeometryCollection), folium raised:This happened because the code assumed every object had a
geometrieskey and didn't handle "bare" geometry objects that store their arcs
directly.
Fix
Updated the TopoJson handling in
folium/features.pyso it alsosupports non-collection geometry types (
Polygon,MultiPolygon,etc.), not just
GeometryCollection.Testing
Reproduced the exact example from #1816 — it rendered without the
KeyError after this change. Ran the existing test suite locally with
python -m pytest tests --ignore=tests/seleniumand all tests pass.