8000 polygonToCells API accepts a flags argument by isaacbrodsky · Pull Request #570 · uber/h3 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

polygonToCells API accepts a flags argument #570

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

Merged
merged 4 commits into from
Feb 4, 2022

Conversation

isaacbrodsky
Copy link
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

Coverage Status

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

H3Index *hexagons = calloc(numHexagons, sizeof(H3Index));

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

int64_t *out) {
uint32_t flags, int64_t *out) {
if (flags != 0) {
return E_FAILED;
Copy link
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
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
0