DRY the CLI#826
Conversation
nrabinowitz
left a comment
There was a problem hiding this comment.
Looks great, thanks for the cleanup!
| if (parseArgs(argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg, \ | ||
| args[0]->helpText)) { \ | ||
| return E_SUCCESS; \ | ||
| } |
There was a problem hiding this comment.
Nit: Do we have a .h header file for this? Should we? If we did, should this macro go there?
There was a problem hiding this comment.
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.
| Arg cellToBoundaryArg = { | ||
| .names = {"cellToBoundary"}, | ||
| .helpText = "Convert an H3 cell to a WKT POLYGON defining its boundary", | ||
| }; |
There was a problem hiding this comment.
Total nit: I'd put these next to the functions that implement them, but 🤷
There was a problem hiding this comment.
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 err = H3_EXPORT(cellToLatLng)(cell, &ll); | ||
| if (err) { | ||
| return false; | ||
| return err; |
Based on the discussion with @nrabinowitz in the prior CLI PR. This PR removes duplicate help text and argument types, and turns the arg length trick (and all of the "show subcommand-specific help text" code) into a macro, deleting about twice as much text as it adds.
PS. DRY the CLI does not rhyme.