+
Skip to content

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.

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

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载