fix(cli): ensure unknown commands other than help return exit code 1 #230
Conversation
Signed-off-by: karan-palan <[email protected]>
src/main.cc
Outdated
| )EOF"}; | ||
|
|
||
| auto jsonschema_main(const std::string &program, const std::string &command, | ||
| auto jsonschema_main([[maybe_unused]] const std::string &program, |
There was a problem hiding this comment.
I don't think you need this annotation. program is always used in the help handler!
| << " <command> [arguments...]\n"; | ||
| std::cout << USAGE_DETAILS; | ||
| return EXIT_SUCCESS; | ||
| } else { |
There was a problem hiding this comment.
Can you add one test covering this case and the output? See how we test help for an example
Signed-off-by: karan-palan <[email protected]>
Karan-Palan
force-pushed
the
fix/unknown-commands-exitcode
branch
from
a7e8134 to
4f8e8a2
Compare
|
Did the changes. PTAL |
There was a problem hiding this comment.
Can you assert the actual error output the CLI prints?
test/unknown_command.sh
Outdated
| clean() { rm -rf "$TMP"; } | ||
| trap clean EXIT | ||
|
|
||
| "$1" unknown_command 1> "$TMP/stdout" 2> "$TMP/stderr" || exit_code=$? |
There was a problem hiding this comment.
Also please see how we do it in other files for consistency. i.e. we use uppercase for variables: EXIT_CODE
There was a problem hiding this comment.
Also see the && EXIT_CODE="$?" || EXIT_CODE="$?" pattern we use to avoid the -z cases you have below
test/unknown_command.sh
Outdated
|
|
||
| "$1" unknown_command 1> "$TMP/stdout" 2> "$TMP/stderr" || exit_code=$? | ||
|
|
||
| if [ -z "${exit_code:-}" ] || [ "$exit_code" -eq 0 ]; then |
There was a problem hiding this comment.
If you look at other files, we put then in its own line. In general, please try to copy the current style as much as possible, otherwise we end up with files that have their own style
src/main.cc
Outdated
| return EXIT_SUCCESS; | ||
| } else { | ||
| std::cerr << "error: Unknown command '" << command << "'.\n"; | ||
| std::cerr << "Use '--help' for usage information.\n"; |
There was a problem hiding this comment.
| std::cerr << "Use '--help' for usage information.\n"; | |
| std::cerr << "Use --help for usage information\n"; |
For consistency
src/main.cc
Outdated
| std::cout << USAGE_DETAILS; | ||
| return EXIT_SUCCESS; | ||
| } else { | ||
| std::cerr << "error: Unknown command '" << command << "'.\n"; |
There was a problem hiding this comment.
| std::cerr << "error: Unknown command '" << command << "'.\n"; | |
| std::cerr << "error: Unknown command '" << command << "'\n"; |
For consistency with all other error messages
Signed-off-by: karan-palan <[email protected]>
|
Done the required changes, ptal |
|
Awesome, thanks a lot! |
2 participants
What kind of change does this PR introduce?
Issue Number:
Screenshots/videos: