Implement the cellToBoundary command for the h3 binary #818
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright 2021, 2024 Uber Technologies, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
|
|
@@ -43,19 +43,24 @@ bool cellToLatLngCmd(int argc, char *argv[]) { | |
| Arg cellToLatLngArg = { | ||
| .names = {"cellToLatLng"}, | ||
| .required = true, | ||
| .helpText = "Convert an H3 cell to a WKT POINT coordinate", | ||
| }; | ||
| Arg helpArg = ARG_HELP; | ||
| DEFINE_CELL_ARG(cell, cellArg); | ||
| Arg *args[] = {&cellToLatLngArg, &helpArg, &cellArg}; | ||
| if (parseArgs(argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg, | ||
| "Convert an H3 cell to a WKT POINT coordinate")) { | ||
| return helpArg.found; | ||
| } | ||
| LatLng ll; | ||
| H3Error err = H3_EXPORT(cellToLatLng)(cell, &ll); | ||
| if (err) { | ||
| return false; | ||
| } | ||
| // Using WKT formatting for the output. TODO: Add support for JSON | ||
| // formatting | ||
|
There was a problem hiding this comment. I'm curious about plans for output and input formats here. I assume for output we'd want a
We might also support The parallel question is what inputs we support - accepting these formats as inputs is nicely symmetrical, but more painful than Not saying we should do any of this, but it might be nice to know where we plan to draw the line. I think a good yardstick should be how easy it is to pipe data from a tool like There was a problem hiding this comment. Yes, this can definitely become a rabbit hole, here. There are other things to consider, for instance: should the But also, how much of this can we generalize across these functions, and how much has to be hand-written per sub-command? The more of the latter the less I want to support everything under the sun and just suggest users to do things like That's part of why I was thinking of going with a JSON-based output by default, because of the possibility of mangling the format more easily than any other, but went with WKT since that's what @isaacbrodsky suggested as an expected output. I'm not personally familiar with |
||
| printf("POINT(%.10lf %.10lf)\n", H3_EXPORT(radsToDegs)(ll.lng), | ||
| H3_EXPORT(radsToDegs)(ll.lat)); | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -92,7 +97,7 @@ bool latLngToCellCmd(int argc, char *argv[]) { | |
|
|
||
| Arg *args[] = {&latLngToCellArg, &helpArg, &resArg, &latArg, &lngArg}; | ||
| if (parseArgs( | ||
| argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg, | ||
| "Convert degrees latitude/longitude coordinate to an H3 cell.")) { | ||
| return helpArg.found; | ||
| } | ||
|
|
@@ -102,6 +107,7 @@ bool latLngToCellCmd(int argc, char *argv[]) { | |
| H3Index c; | ||
| H3Error e = H3_EXPORT(latLngToCell)(&ll, res, &c); | ||
|
|
||
| // TODO: Add support for JSON formatting | ||
|
There was a problem hiding this comment. Does JSON formatting mean There was a problem hiding this comment. That or |
||
| if (e == E_SUCCESS) { | ||
| h3Println(c); | ||
| } else { | ||
|
|
@@ -110,23 +116,63 @@ bool latLngToCellCmd(int argc, char *argv[]) { | |
| return true; | ||
| } | ||
|
|
||
| bool cellToBoundaryCmd(int argc, char *argv[]) { | ||
| Arg cellToBoundaryArg = { | ||
| .names = {"cellToBoundary"}, | ||
| .required = true, | ||
| .helpText = "Convert an H3 cell to a WKT POLYGON defining its boundary", | ||
| }; | ||
| Arg helpArg = ARG_HELP; | ||
| DEFINE_CELL_ARG(cell, cellArg); | ||
| Arg *args[] = {&cellToBoundaryArg, &helpArg, &cellArg}; | ||
| if (parseArgs( | ||
| argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg, | ||
| "Convert an H3 cell to a WKT POLYGON defining its boundary")) { | ||
| return helpArg.found; | ||
| } | ||
| CellBoundary cb; | ||
| H3Error err = H3_EXPORT(cellToBoundary)(cell, &cb); | ||
| if (err) { | ||
| return false; | ||
| } | ||
| // Using WKT formatting for the output. TODO: Add support for JSON | ||
| // formatting | ||
| printf("POLYGON(("); | ||
| for (int i = 0; i < cb.numVerts; i++) { | ||
| LatLng *ll = &cb.verts[i]; | ||
| printf("%.10lf %.10lf, ", H3_EXPORT(radsToDegs)(ll->lng), | ||
| H3_EXPORT(radsToDegs)(ll->lat)); | ||
| } | ||
| // WKT has the first and last points match, so re-print the first one | ||
| printf("%.10lf %.10lf))\n", H3_EXPORT(radsToDegs)(cb.verts[0].lng), | ||
| H3_EXPORT(radsToDegs)(cb.verts[0].lat)); | ||
|
Comment on lines
+147
to
+148
There was a problem hiding this comment. nit/note if this function gets reused, that it depends on numVerts > 0. There was a problem hiding this comment. Is it possible for it to be zero and not produce an H3Error value that short-circuits it, though? There was a problem hiding this comment. As far as I know it should always have vertices if the return code is success |
||
| return true; | ||
| } | ||
|
|
||
| bool generalHelp(int argc, char *argv[]) { | ||
| Arg helpArg = ARG_HELP; | ||
| Arg cellToLatLngArg = { | ||
| .names = {"cellToLatLng"}, | ||
| .helpText = "Convert an H3 cell to a WKT POINT coordinate", | ||
| }; | ||
| Arg latLngToCellArg = { | ||
| .names = {"latLngToCell"}, | ||
| .helpText = | ||
| "Convert degrees latitude/longitude coordinate to an H3 cell.", | ||
| }; | ||
| Arg cellToBoundaryArg = { | ||
| .names = {"cellToBoundary"}, | ||
| .helpText = "Convert an H3 cell to a WKT POLYGON defining its boundary", | ||
| }; | ||
|
There was a problem hiding this comment. Nit: Is it possible to define this a top-level There was a problem hiding this comment. See my prior comment. Leaving DRY-ing it to another PR focused solely on that so we can better debate the merits/long-term maintenance. |
||
| Arg *args[] = {&helpArg, &cellToLatLngArg, &latLngToCellArg, | ||
| &cellToBoundaryArg}; | ||
|
|
||
| const char *helpText = | ||
| "Please use one of the subcommands listed to perform an H3 " | ||
| "calculation. Use h3 <SUBCOMMAND> --help for details on the usage of " | ||
| "any subcommand."; | ||
| return parseArgs(argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg, | ||
| helpText); | ||
| } | ||
|
|
||
| int main(int argc, char *argv[]) { | ||
|
|
@@ -140,6 +186,9 @@ int main(int argc, char *argv[]) { | |
| if (has("latLngToCell", 1, argv) && latLngToCellCmd(argc, argv)) { | ||
| return 0; | ||
| } | ||
| if (has("cellToBoundary", 1, argv) && cellToBoundaryCmd(argc, argv)) { | ||
| return 0; | ||
|
There was a problem hiding this comment. As above, I would have gone with There was a problem hiding this comment. I agree with you, but would like that to also be a separate PR. |
||
| } | ||
| if (generalHelp(argc, argv)) { | ||
| return 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.
For my info, why return
true/falsefrom these functions? Wouldn't we just want to return the error code, so callers would get a0exit code on success and a meaningful non-zero code on failure?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.
So, I think we started work on this binary before
H3Errorwas a thing and it was doingh3IsValidchecks on the inputs/outputs to determine if it should exit with an error or not.Then the conversion of that was done this way because it was the most straightforward. Bubbling up the
H3Errordoes make sense to me. If you don't mind, I'll keep that refactor for when I add in the next sub-command.