Skip to content

Add LinkedGeo <-> GeoMultiPolygon conversion functions#1126

Merged
ajfriend merged 10 commits intouber:masterfrom
ajfriend:aj/poly_convert
Mar 2, 2026
Merged

Add LinkedGeo <-> GeoMultiPolygon conversion functions#1126
ajfriend merged 10 commits intouber:masterfrom
ajfriend:aj/poly_convert

Conversation

@ajfriend
Copy link
Collaborator

@ajfriend ajfriend commented Feb 15, 2026

  • Add internal functions linkedGeoPolygonToGeoMultiPolygon and geoMultiPolygonToLinkedGeoPolygon for converting between the two polygon representations
  • Both directions validate geometry: every loop must have >= 3 vertices and every polygon must have an outer loop (E_FAILED otherwise). Empty multi-polygons (0 polygons) are allowed.
  • Changes should have 100% line and branch coverage.
  • Part of Replace cellsToLinkedMultiPolygon implementation with cellsToMultiPolygon #1127

@coveralls
Copy link

coveralls commented Feb 15, 2026

Coverage Status

coverage: 99.095% (+0.08%) from 99.013%
when pulling b9f33ed on ajfriend:aj/poly_convert
into 8085979 on uber:master.

Copy link
Collaborator

@isaacbrodsky isaacbrodsky left a comment

Choose a reason for hiding this comment

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

I would err on the side of keeping these as internal functions rather than exposed, but as they would not be required for the bindings maybe it doesn't matter too much?

Edit: we can of course add fuzzing targets even for internal functions :)

@ajfriend
Copy link
Collaborator Author

I would err on the side of keeping these as internal functions rather than exposed, but as they would not be required for the bindings maybe it doesn't matter too much?

I agree that we don't need to be as strict about these conversion functions: 1) They wouldn't be exposed to users of the bindings. 2) These are really only for backwards compatibility, and we'll be recommending that people use cellsToMultiPolygon going forward. So, ideally, users and bindings would never need to touch (or convert to/from) LinkedGeoPolygon.

That being said, if a user does need to convert between them, I think providing the utilities to do so is helpful, rather than having them rewrite it themselves (like we historically did in the bindings).

@ajfriend
Copy link
Collaborator Author

Agreed with @isaacbrodsky that we can have these functions be internal for now. It would be easy to expose them in the future if desired.

@ajfriend
Copy link
Collaborator Author

Updated based on comments, and code coverage stays at 100% for changes.

@ajfriend ajfriend modified the milestones: v4.6, v4.5 Feb 25, 2026
Copy link
Contributor

@justinhwang justinhwang left a comment

Choose a reason for hiding this comment

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

lgtm!

@ajfriend ajfriend merged commit 8ac991e into uber:master Mar 2, 2026
89 of 92 checks passed
@ajfriend ajfriend deleted the aj/poly_convert branch March 2, 2026 17:55
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.

4 participants