Skip to content

polygonToCells API accepts a flags argument#570

Merged
isaacbrodsky merged 4 commits into
uber:masterfrom
isaacbrodsky:polyfill-flags-api
Feb 4, 2022
Merged

polygonToCells API accepts a flags argument#570
isaacbrodsky merged 4 commits into
uber:masterfrom
isaacbrodsky:polyfill-flags-api

Conversation

@isaacbrodsky
Copy link
Copy Markdown
Collaborator

This supersedes and includes #519 and #549, but includes only the API changes, but not any specific implementations.

Isaac Brodsky added 2 commits February 1, 2022 17:37
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.003%) to 98.653% when pulling 7e43a20 on isaacbrodsky:polyfill-flags-api into adde6da on uber:master.

resetMemoryCounters(0);
failAlloc = true;
H3Error err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons);
H3Error err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons);
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.

Nit: Would prefer that tests use a const like DEFAULT_FLAGS instead of 0 - it makes the call clearer and it will be easier to update when we have meaningful input to test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The value 0 will be part of the API so we will not be modifying the value of DEFAULT_FLAGS. I can certainly add a constant for that. In the future when polygonToCells accepts more flags, we'll need to offer appropriate constants / defines for users to OR together.

Comment thread src/h3lib/lib/algos.c
int64_t *out) {
uint32_t flags, int64_t *out) {
if (flags != 0) {
return E_FAILED;
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.

Worth adding E_UNSUPPORTED_FLAGS or similar?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm fine either way. I don't believe the specific error code is considered part of the API contract so I don't see an issue with it changing.

@isaacbrodsky isaacbrodsky merged commit 28b7107 into uber:master Feb 4, 2022
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