-
Notifications
You must be signed in to change notification settings - Fork 446
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
Implement the traversal subcommands for the H3 cli #839
Conversation
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.
please note we will need to add tests, docs and fuzzer (or document its exemption, should never be a problem) for describeH3Error since it is in the public API.
if (err) { | ||
return err; | ||
} | ||
H3Index *out = calloc(len, sizeof(H3Index)); |
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.
Do we need to check for failed memory allocation here?
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.
Technically yes, but the app is going to die if this happens no matter what if this happens. But I'd also assume anybody on a machine that cannot allocate 8 bytes is having a much harder time, which is why I left that error handling out?
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.
This could be much larger than 8 bytes with larger values of k
. I think the program is better behaved if it explicitly tells the user that memory allocation failed rather than risk what happens with segfaulting, as in that case the user would have a much harder time tracking down what happened (they would need to attach a debugger to find out the problem is the k
value.)
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.
Addressed. There's now a print to stderr
and an exit
for each if the pointer is NULL
.
.valueName = "distance", | ||
.value = &k, | ||
.helpText = "Maximum grid distance for the output set"}; | ||
Arg prettyArg = { |
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.
FWIW, I'd skip - interested callers have access to plenty of other pretty-printing tools
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.
Probably true. But I wasn't sure if we should rely on the users having jq
, especially if they're more on the traditional geospatial/data analyst side of things and less familiar with the terminal.
// Since we don't know *actually* how many cells are in the output (usually | ||
// the max, but sometimes not), we need to do a quick scan to figure out the | ||
// true length in order to properly serialize to a JSON array | ||
int64_t trueLen = 0; |
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.
This is all for comma handling, right? Could this be simplified if you output a leading comma, not a trailing one, for every cell but out[0]
?
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.
I can't do that because I can't assume out[0]
has a value.
That's what I originally had until @isaacbrodsky pointed out my mistake there.
if (err) { | ||
return err; | ||
} | ||
H3Index *out = calloc(len, sizeof(H3Index)); |
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.
This could be much larger than 8 bytes with larger values of k
. I think the program is better behaved if it explicitly tells the user that memory allocation failed rather than risk what happens with segfaulting, as in that case the user would have a much harder time tracking down what happened (they would need to attach a debugger to find out the problem is the k
value.)
@@ -13,56 +13,57 @@ The public API of H3 is covered in the following fuzzers: | |||
|
|||
| Function | File or status | |||
| -------- | -------------- | |||
| latLngToCell | [fuzzerLatLngToCell](./fuzzerLatLngToCell.c) | |||
| cellToLatLng | [fuzzerCellToLatLng](./fuzzerCellToLatLng.c) | |||
| areNeighborCells | [fuzzerDirectedEdge](./fuzzerDirectedEdge.c) |
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.
for my curiosity - was this an editor feature you used to sort this? or did you manually alphabetize?
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.
Simple :sort
in vim after highlighting the lines I wanted to sort. :)
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.
I figured it must be some vim thing :)
Co-authored-by: Isaac Brodsky <isaac@isaacbrodsky.com>
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.
LGTM!
This implements all of the traversal subcommands.
The test suite has not been implemented yet, as I intend to rework the way CLI tests are structured, as requested by @nrabinowitz.
But I'm putting this up now before I start on that in case there are any changes to the input and output formats to be done before I start codifying them into the tests.