Skip to content

fix: replace geo input assert with ValueError and add test - #502

Open
dyrpsf wants to merge 3 commits into
uber:masterfrom
dyrpsf:fix-geo-type-validation
Open

fix: replace geo input assert with ValueError and add test#502
dyrpsf wants to merge 3 commits into
uber:masterfrom
dyrpsf:fix-geo-type-validation

Conversation

@dyrpsf

@dyrpsf dyrpsf commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Description

This PR resolves a lingering todo comment in src/h3/_h3shape.py.

The geo_to_h3shape function contained a debug assertion (assert isinstance(geo, dict) # todo: remove) that was explicitly marked for removal.

To improve robustness, I replaced the raw assertion with a proper ValueError check to prevent confusing TypeError crashes if a user accidentally passes a string. I also added a unit test to test_polyfill.py to ensure this specific bad input is caught correctly.

Maintainers

cc: @dfellis @ajfriend @isaacbrodsky

@dyrpsf

dyrpsf commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

I noticed the assert in geo_to_h3shape was recently updated to a ValueError on master, but there wasn't a specific test for it. This PR adds a unit test to test_polyfill.py to ensure this ValueError correctly catches invalid string inputs.

@dyrpsf

dyrpsf commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

I've resolved the merge conflicts and fixed the CI linter error!

Just to recap: it looks like the ValueError fix in _h3shape.py was recently merged into master, so I accepted those incoming changes during the conflict resolution. This PR now strictly adds the missing unit test to test_polyfill.py to ensure that specific ValueError is properly tested against invalid string inputs.

The tests should be completely green now and ready for review!

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.

1 participant