Skip to content

Feat: Add explicit container options for GeoJSON outputs#495

Open
dyrpsf wants to merge 4 commits into
uber:masterfrom
dyrpsf:feature-geojson-containers
Open

Feat: Add explicit container options for GeoJSON outputs#495
dyrpsf wants to merge 4 commits into
uber:masterfrom
dyrpsf:feature-geojson-containers

Conversation

@dyrpsf

@dyrpsf dyrpsf commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Overview
This PR implements the TODO noted in _h3shape.py regarding allowing users to specify strict GeoJSON containers when converting cells to geometries.

Changes Made
Updated h3shape_to_geo to accept a container argument. By default, it retains the 'auto' behavior (returning the simplest valid Polygon or MultiPolygon). However, it now supports wrapping the output into specific GeoJSON structures:

  • MultiPolygon (safely wraps a standard Polygon)
  • Feature
  • FeatureCollection
  • GeometryCollection

This is particularly useful for downstream pipelines (like mapping libraries or databases) that require strict schema enforcement, such as always expecting a FeatureCollection even if the query only returns a single cell.

cc: @jongbinjung, @dfellis, @isaacbrodsky

@isaacbrodsky isaacbrodsky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks - the implementation looks good to me.

Could you add some tests to demonstrate this?

@dyrpsf

dyrpsf commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks - the implementation looks good to me.

Could you add some tests to demonstrate this?

Thanks @isaacbrodsky! I've added a suite of unit tests in tests/test_geojson_containers.py to verify all the new wrapper formats. I also resolved the trailing whitespace issue that was tripping up ruff in the CI pipeline. Let me know if you need anything else!

Comment thread src/h3/_h3shape.py Outdated
@dfellis
dfellis requested a review from ajfriend July 22, 2026 14:30
@dyrpsf
dyrpsf requested review from dfellis and isaacbrodsky July 22, 2026 15:12
Comment thread src/h3/_h3shape.py
@dyrpsf
dyrpsf requested a review from dfellis July 22, 2026 17:18
@dyrpsf
dyrpsf force-pushed the feature-geojson-containers branch from 61de89c to 5404d22 Compare July 22, 2026 17:33
@dfellis

dfellis commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

@dyrpsf unfortunately after approving the test suite run, not only is the linter still failing, but there are failing tests, as well: https://github.com/uber/h3-py/actions/runs/29942829160/job/89001681729?pr=495#step:8:510

@dyrpsf

dyrpsf commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@dyrpsf unfortunately after approving the test suite run, not only is the linter still failing, but there are failing tests, as well: https://github.com/uber/h3-py/actions/runs/29942829160/job/89001681729?pr=495#step:8:510

Thanks for the heads-up, @dfellis! I see the failed checks for both the linter and the test suite. I'm reproducing the errors locally right now so I can track down exactly what broke. I'll have a fix pushed shortly!

@dyrpsf
dyrpsf force-pushed the feature-geojson-containers branch from 5404d22 to dd96a09 Compare July 23, 2026 06:49
@dyrpsf
dyrpsf force-pushed the feature-geojson-containers branch from dd96a09 to a2add15 Compare July 23, 2026 06:56
@dyrpsf

dyrpsf commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Hey @dfellis, I just force-pushed the fixes! I added the explicit ValueError logic for invalid/insufficient containers, handled the exact-match scenario, and added unit tests to verify the behaviour. I also resolved the formatting issues so ruff is passing. Since I don't have permissions to trigger the CI workflows, could you please hit 'Approve and run' when you get a chance? Thanks!

@dfellis

dfellis commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Hey @dfellis, I just force-pushed the fixes! I added the explicit ValueError logic for invalid/insufficient containers, handled the exact-match scenario, and added unit tests to verify the behaviour. I also resolved the formatting issues so ruff is passing. Since I don't have permissions to trigger the CI workflows, could you please hit 'Approve and run' when you get a chance? Thanks!

Was asleep when you sent that. Just triggered it.

@dfellis

dfellis commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

@dyrpsf so @ajfriend was very strict when he set up this repo :) https://github.com/uber/h3-py/actions/runs/29986675613/job/89269426447?pr=495#step:8:38

The test suite passes, but now it's no longer 100% code coverage, so you need more tests.

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