Skip to content
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

Integer array API #40

Merged
merged 6 commits into from
Oct 23, 2021
Merged

Integer array API #40

merged 6 commits into from
Oct 23, 2021

Conversation

isaacbrodsky
Copy link
Contributor

Applies the change in uber/h3-js#91 to h3-node. There is a slight difference that invalid index objects cause the library to throw instead of returning false from h3IsValid; not sure if we want to bring that into consistency. I'm sure we could also add better testing for this change, perhaps test that h3ToGeo behaves the same with both forms of input?

test/index.js Outdated
'Integer with incorrect bits is not considered an index'
);
// TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing
test.throws(() => h3node.h3IsValid([]), 'Empty array is not valid');

Choose a reason for hiding this comment

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

This seems like incorrect behavior. Is there a way to wrap in a try/catch? Are we getting these errors from your new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, although these cases will all throw in h3-node as it is. We would need to change the behavior of h3-node to match h3-js here. Further, these cases will throw in other functions, too, unlike in h3-js where [0, 0] is used.

Choose a reason for hiding this comment

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

I guess this is up to @dfellis, as it's an API question, but my assumption is that h3IsValid should never throw for invalid input, since it's the job of the function to recognize it. We could check for valid C input in JS and return false before sending to C and getting an error.

Copy link
Owner

Choose a reason for hiding this comment

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

So these bindings directly expose the functions without a Javascript wrapper and that would go against the perf angle for this repo.

This particular C function seems like it would be relatively trivial to replace throwing an error with returning false, while some of the others would be more difficult.

Is the objection primarily for h3IsValid or for any deviation wrt h3-js? The former I think @isaacbrodsky or myself could quickly fix in this PR, the latter I would prefer to happen in a different PR.

Choose a reason for hiding this comment

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

I was just thinking of h3IsValid, because presumably folks might use it to check untrusted data before passing it to another H3 function. In general, I'd expect a validator to either return false or throw, but not both for different cases, and particularly with the is- prefix it seems like it should never throw on bad data. As a developer, I'd much prefer something like indexes.filter(h3IsValid) than

indexes.filter(idx => {
  try {
    return h3IsValid(idx);
  } catch (e) {
    return false;
  }
})

h3napi.c Outdated
H3Index O = stringToH3(O ## Str);
if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\
O = stringToH3(O ## Str);\
} else {\
Copy link
Owner

Choose a reason for hiding this comment

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

can you make this an else if checking for whether or not the input is an array and then restore the error message above for the else path (now saying must be a string or pair of 32-bit integers in an array)?

That would also make it amenable to eventually supporting BigInt inputs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored to make that flow clearer.

napi_throw_error(env, "EINVAL", "Expected string h3 index in arg " #I);\
if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\
O = stringToH3(O ## Str);\
} else if (napi_get_typedarray_info(env, O ## Arr, &O ## Type, &O ## Length, &O ## Data, NULL, NULL) == napi_ok) {\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think NULL is fine here: nodejs/node#40371

@dfellis
Copy link
Owner

dfellis commented Oct 22, 2021

Will wait for @nrabinowitz to also approve and then I will merge it.

test/index.js Outdated
@@ -113,6 +113,64 @@ const exportTest = (methodName, genArgs, testFn, extraName = '') =>
const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = '') =>
exports[`${methodName}${extraName}Benchmark`] = benchmarkGen(methodName, genArgs, useTryCatch, extraName)

// h3IsValid has unique input parsing logic to return false rather than throw on invalid input
exports['h3IsValid_array'] = test => {
test.ok(h3node.h3IsValid([0x3fffffff, 0x8528347]), 'Integer H3 index is considered an index');

Choose a reason for hiding this comment

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

Nit: For methods expected to return boolean, prefer t.equal(result, false) etc to avoid tests passing on truthy/falsy values like undefined etc.

@dfellis dfellis merged commit 6b55ff1 into dfellis:master Oct 23, 2021
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.

3 participants