-
Notifications
You must be signed in to change notification settings - Fork 470
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
Rename h3SetToLinkedGeo to cellsToLinkedMultiPolygon #571
Conversation
e52f8fd
to
04f3cac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are FAST
Partially automated refactoring :) |
| *DNE* | `loopToBoundary` | | | ||
| *DNE* | `boundaryToLoop` | | | ||
| `getH3UnidirectionalEdgeBoundary` | `directedEdgeToBoundary` | returns `CellBoundary` | | ||
| `h3SetToLinkedGeo` | `cellsToLinkedMultiPolygon` | returns `LinkedGeoPolygon` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also rename LinkedGeoPolygon
? This struct is part of the external API, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per line 269 it looks like the rename should be LinkedGeoMultiPolygon
? It's part of the external C API in h3api.h
, although it is not needed for binary compatability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldn't be necessary. The linked structures have a next pointer. So instead of a MultiPolygon with Polygon children, you have a Polygon that can optionally define a pointer to a Polygon sibling.
Maybe the rename isn't necessary - I thought we were dropping the Geo
in GeoPolygon
, so I was suggesting this should follow suit. But if not, this is fine - though I guess we could update LinkedGeoCoord
to LinkedLatLng
for consistency.
Rename for consistency with the naming RFC. The name "LinkedMultiPolygon" was chosen so that in the future a "ToMultiPolygon" version could be added; multi was needed since this is inherently a multipolygon as differentiated from polygonToCells which deals only with polygons (non-multi)