Add argument parsing #224
Add argument parsing #224
Conversation
src/apps/applib/lib/utility.c
Outdated
|
|
||
| for (int j = 0; j < numArgs; j++) { | ||
| const char* argName = NULL; | ||
| bool isMatch = false; |
There was a problem hiding this comment.
Nit: You could just check argName == NULL and not need isMatch, right?
There was a problem hiding this comment.
Sure, I don't strongly feel isMatch is needed.
src/apps/applib/lib/utility.c
Outdated
| return 1; | ||
| } | ||
|
|
||
| if (args[j].valuePresent != NULL) { |
There was a problem hiding this comment.
Sorry, feeling dumb here - what's being set here? Possibly deserves a comment.
There was a problem hiding this comment.
valuePresent is an optional pointer. If set, parseArgs will set what it points to to true if the argument is found.
There was a problem hiding this comment.
Ok, got it. Maybe clarify the comment in the struct def?
| *errorDetail = strdup(argName); | ||
| return 3; | ||
| } | ||
| } |
There was a problem hiding this comment.
Error if we're not expecting a value but we get one? Or will that just throw when we try to parse the next argument?
There was a problem hiding this comment.
I'm not sure we could reliably differentiate that from another argument being present next. It will hit an error condition when the next argument is parsed.
src/apps/applib/lib/utility.c
Outdated
| free(foundArgs); | ||
| *errorMessage = strdup("Unknown argument"); | ||
| // Don't set errorDetail, since the input could be unprintable. | ||
| return 4; |
There was a problem hiding this comment.
Is there value to named constants for these error return vals?
There was a problem hiding this comment.
Possibly not for parsing the return codes (considering errorMessage and errorDetails), but for code readability I'd say yes.
src/apps/applib/lib/utility.c
Outdated
| } | ||
| } | ||
|
|
||
| free(foundArgs); |
There was a problem hiding this comment.
It seems like you could avoid foundArgs and skip this memory management if you had an additional found field on the Arg struct.
There was a problem hiding this comment.
Changed, this is now on the Arg struct.
src/apps/filters/kRing.c
Outdated
| char* errorDetails = NULL; | ||
| if (parseArgs(argc, argv, 4, args, &errorMessage, &errorDetails) || | ||
| helpArg) { | ||
| printHelp(helpArg ? stdout : stderr, argv[0], |
There was a problem hiding this comment.
Should this "print help if help arg or on arg parse failure" behavior just be built into parseArgs?
There was a problem hiding this comment.
It would certainly deduplicate this code, which will need to be in all filter applications. The trade-off is this places another responsibility in parseArgs, possibly making parseArgs harder to test as a unit, and I would prefer to avoid calling exit since it requires knowing parseArgs may never return.
There was a problem hiding this comment.
Agreed that parseArgs shouldn't call exit, but I think the functionality of printing out help messages is pretty standard in arg parsing libs, and you could avoid both the printHelp call and (I think?) the memory management for errorMessage and errorDetails here if they were included in parseArgs - then this would simply read
if (parseArgs(argc, argv, 4, args) || helpArg) {
return helpArg ? 0 : 1;
}
which would be much simpler and easier to deal with in the filters. Given that we'll need this in every filter, I think the code reduction and reduced maintenance make this totally worthwhile.
There was a problem hiding this comment.
Good point, I refactored the code for printing help into utility.c
There was a problem hiding this comment.
This looks much cleaner IMO - thanks!
src/apps/applib/lib/utility.c
Outdated
| * than once. | ||
| * | ||
| * Help is printed to stdout if a argument with isHelp = true is found, and help | ||
| * si printed to stderr if argument parsing fails. |
Add argument parsing
Changes the
kRingfilter to use a new options parsing function. This is only applied tokRingat first to discuss the argument parsing itself, once this is OK'd it can be expanded to the other filter applications.