-
Notifications
You must be signed in to change notification settings - Fork 75
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
#1682: fix help tests #1734
base: main
Are you sure you want to change the base?
#1682: fix help tests #1734
Conversation
19d4abb
to
7453642
Compare
7453642
to
e4b3254
Compare
e4b3254
to
2b58f20
Compare
What needs to merge before this PR can be merged? |
@leighmcculloch we can either merge all listed PRs at the same time or merge all of them here first |
This is looking great! Thanks for digging into it - it was driving me crazy ;p
W/r/t when to merge this and the other PRs, i like the idea of merging those test fixing into this PR first. |
Co-authored-by: Elizabeth Engelman <[email protected]>
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.
Thanks for figuring out why these tests were preventing from running. Great find and learning for us all.
For the solution, I'd like us to reconsider the approach here, see the inline comment. Please lmk if there's a reason we can't do the error approach.
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.
One ask, otherwise this looks good, very easy to follow.
} | ||
}, | ||
Err(ArgParse(HelpMessage(help))) => { | ||
println!("{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.
I think we need to do this at the top level, rather than inside the arg parsing, otherwise the exit codes will be incorrect when generating the help when no --help
is passed. It's a small thing, but also I believe a small fix to keep passing the error as is up the chain, and at the top level doing as you do here.
Err(ArgParsing(HelpMessage(help))) => { | ||
println!("{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.
Same comment about doing this at the top level.
Part of #1682
In this PR
help
tests are fixed.As part of the change, I re-organized
build_host_function_parameters
to return either previous result or help message. All callers up the stack now respect this behavior.Why
Previously, we were using clap's
get_matches_from
that exits with code 0 (printing help message). This made our tests exit with code 0 as well (when calling--help
).As part of the change we handle
DisplayError
returning help message, and keeping existing behavior for all other clap errors.Known limitations
Some of the tests are now failing in CI. They were previously effectively ignored, as the whole suite was exiting as soon as help tests were running.
This tests are fixed in other PRs (see #1682 )