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.

Sign up for GitHub

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

DRY the CLI #826

Merged
merged 2 commits into from
May 3, 2024
Merged

DRY the CLI #826

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d6f752
DRY the CLI
dfellis Mar 29, 2024
ea7489f
Address comment by @isaacbrodsky
dfellis May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 39 additions & 62 deletions src/apps/filters/h3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,47 +35,51 @@
#include "h3Index.h"
#include "utility.h"

#define PARSE_SUBCOMMAND(argc, argv, args) \
if (parseArgs(argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg, \
args[0]->helpText)) { \
return E_SUCCESS; \
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Do we have a .h header file for this? Should we? If we did, should this macro go there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We kinda do, but it's shared across many of these binaries while this is only useful here.

On top of that, if I put it in there, I can't make the assumption about the naming of the various variables in use nor that the first argument in the array is declaring the sub-command name with the help text attached, so much of what makes it "magic" here would go away. It would just be a parseArgs function that does the sizeof(args) / sizeof(Arg *) trick for you, and not much else.


bool has(char *subcommand, int level, char *argv[]) {
return strcasecmp(subcommand, argv[level]) == 0;
}

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;
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",
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Total nit: I'd put these next to the functions that implement them, but 🤷

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 waffled back and forth on that. As long as the generalHelp and main functions are the last in the file, that should work just fine, but it might be less obvious why they aren't defined within their function body, so I thought grouping them together as a collection of global variables would at least get people to pause before immediately wanting to refactor it and accidentally break the generalHelp function.

I was also thinking of turning the sub-command function declaration into a macro that makes these global variables at the same time it makes the sub-command function, but I don't know how "magic" we want the syntax in this file to be, so I didn't go that route.


H3Error cellToLatLngCmd(int argc, char *argv[]) {
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;
}
PARSE_SUBCOMMAND(argc, argv, args);
LatLng ll;
H3Error err = H3_EXPORT(cellToLatLng)(cell, &ll);
if (err) {
return false;
return err;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

}
// Using WKT formatting for the output. TODO: Add support for JSON
// formatting
printf("POINT(%.10lf %.10lf)\n", H3_EXPORT(radsToDegs)(ll.lng),
H3_EXPORT(radsToDegs)(ll.lat));
return true;
return E_SUCCESS;
}

bool latLngToCellCmd(int argc, char *argv[]) {
H3Error latLngToCellCmd(int argc, char *argv[]) {
int res = 0;
double lat = 0;
double lng = 0;

Arg latLngToCellArg = {
.names = {"latLngToCell"},
.required = true,
.helpText =
"Convert degrees latitude/longitude coordinate to an H3 cell.",
};
Arg helpArg = ARG_HELP;
Arg resArg = {.names = {"-r", "--resolution"},
.required = true,
.scanFormat = "%d",
Expand All @@ -96,11 +100,7 @@ bool latLngToCellCmd(int argc, char *argv[]) {
.helpText = "Longitude in degrees."};

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;
}
PARSE_SUBCOMMAND(argc, argv, args);
LatLng ll = {.lat = H3_EXPORT(degsToRads)(lat),
.lng = H3_EXPORT(degsToRads)(lng)};

Expand All @@ -111,29 +111,20 @@ bool latLngToCellCmd(int argc, char *argv[]) {
if (e == E_SUCCESS) {
h3Println(c);
} else {
h3Println(H3_NULL);
h3Println(
H3_NULL); // TODO: Should we print a better error message here?
}
return true;
return e;
}

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;
H3Error cellToBoundaryCmd(int argc, char *argv[]) {
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;
}
PARSE_SUBCOMMAND(argc, argv, args);
CellBoundary cb;
H3Error err = H3_EXPORT(cellToBoundary)(cell, &cb);
if (err) {
return false;
return err;
}
// Using WKT formatting for the output. TODO: Add support for JSON
// formatting
Expand All @@ -146,24 +137,10 @@ bool cellToBoundaryCmd(int argc, char *argv[]) {
// 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));
return true;
return E_SUCCESS;
}

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",
};
Arg *args[] = {&helpArg, &cellToLatLngArg, &latLngToCellArg,
&cellToBoundaryArg};

Expand All @@ -180,14 +157,14 @@ int main(int argc, char *argv[]) {
printf("Please use h3 --help to see how to use this command.\n");
return 1;
}
if (has("cellToLatLng", 1, argv) && cellToLatLngCmd(argc, argv)) {
return 0;
if (has("cellToLatLng", 1, argv)) {
return cellToLatLngCmd(argc, argv);
}
if (has("latLngToCell", 1, argv) && latLngToCellCmd(argc, argv)) {
return 0;
if (has("latLngToCell", 1, argv)) {
return latLngToCellCmd(argc, argv);
}
if (has("cellToBoundary", 1, argv) && cellToBoundaryCmd(argc, argv)) {
return 0;
if (has("cellToBoundary", 1, argv)) {
return cellToBoundaryCmd(argc, argv);
}
if (generalHelp(argc, argv)) {
return 0;
Expand Down
Loading