-
Notifications
You must be signed in to change notification settings - Fork 470
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
Add argument parsing for other filter applications #238
Conversation
"Resolution, if less than the resolution of the prefix " | ||
"only the prefix is printed. Default 0."}; | ||
Arg prefixArg = { | ||
.names = {"-p", "--prefix"}, |
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.
Why is this called prefix
instead of parent
?
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.
Not sure why I did that - I thought perhaps the term prefix
had been used in these filters before but I see from the diff that isn't the case. I changed it to parent
.
We might want to refactor these to h3ToParent
/h3ToChildren
/... to match the C API later.
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 all of these app changes could be considered a breaking change requiring a major version bump (if there's anyone out there who has a shell script dependent on them) but I don't think we've considered them part of the API before.
We could make that change if/when we decide to do that (and presumably after we make sure the app collection is capable of doing everything the C API can?)
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 all of these app changes could be considered a breaking change requiring a major version bump (if there's anyone out there who has a shell script dependent on them) but I don't think we've considered them part of the API before.
We are not considering the filter applications part of the public API at this time.
We could make that change if/when we decide to do that (and presumably after we make sure the app collection is capable of doing everything the C API can?)
Yes, I mention it to point out an existing terminology which isn't consistent with the C API.
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 except for the weird terminology change versus the core API. Prefix instead of parent in a few of the applications.
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.
Looks good overall. I've got a few comments that apply to most of the filters; several of these are suggestions and you should feel free to ignore or defer them.
} | ||
printf("\n"); | ||
} else { | ||
printf("INVALID INDEX\n"); |
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.
Doesn't look like we check h3IsValid
here, just the mode - should we check if the input is valid here? Not sure if that's necessary for all the filters, but almost certainly necessary for this one (plus this filter doesn't have perf concerns).
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 a good point that all filters should handle invalid input, but I am going to defer this to a later change.
src/apps/filters/h3ToComponents.c
Outdated
.valueName = "index", | ||
.value = &index, | ||
.helpText = | ||
"Index, or not specified to read indexes from standard in."}; |
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.
Nit: stdin
is actually clearer IMO. I think the long form is "standard input".
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.
Sampling a few man
pages, the long form does seem to be "standard input" and "stdin" doesn't seem to be used in text.
else | ||
error("reading H3 index from stdin"); | ||
} | ||
if (!H3_EXPORT(h3IsValid)(origin)) { |
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.
Same note - we should probably fail on invalid index
or stdin
input as well.
src/apps/filters/h3ToLocalIj.c
Outdated
|
||
H3Index origin; | ||
Arg helpArg = {.names = {"-h", "--help"}, |
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 a total nit, but declaring each arg as a variable like this leads to inconsistent indentation, which makes the arg list far less legible.
I think this might read better as:
Arg* args = [
{.names = {"-h", "--help"},
...
];
// Then name the args
Arg helpArg = args[0];
This avoids naming args you don't need to reference as vars, and keeps the indentation consistent.
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 am not that convinced, I think that indexing into the args array could lead to errors (off by one, miscounting the number of arguments, etc).
src/apps/miscapps/h3ToGeoHier.c
Outdated
.value = &res, | ||
.helpText = | ||
"Resolution, if less than the resolution of the parent " | ||
"only the parent is printed. Default 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.
Nit: Default 0
seems incorrect; the default is the res of the parent.
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 program accepts coarser resolutions as valid input, so this is documenting that existing behavior. From that perspective, 0 does seem to really be the default. I changed it to be a little clearer.
if (argc > 3) { | ||
if (!sscanf(argv[3], "%d", &isKmlOut)) | ||
error("outputMode must be an integer"); | ||
if (kmlArg.found) { |
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 block also seems like it could be defined in one place and reused.
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.
A few small comments, but LGTM overall. Fine to leave validation until a future PR.
Add argument parsing for other filter applications
Adds argument parsing for other applications, except for test/generator applications. This should bring argument parsing to all the existing filter applications.