From d67ce423d1d82edc765719bf482cc1fdbb78c8e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Mangano-Tarumi?= Date: Sat, 1 Dec 2018 13:26:22 -0500 Subject: [PATCH] parse_options: return the error message in the status --- src/cli.cc | 57 +++++++++++++++++--------------------------------- src/opustags.h | 2 -- t/cli.cc | 24 +++++++++++++++------ t/opustags.t | 4 ++-- 4 files changed, 39 insertions(+), 48 deletions(-) diff --git a/src/cli.cc b/src/cli.cc index cb14c09..8f1a21a 100644 --- a/src/cli.cc +++ b/src/cli.cc @@ -59,7 +59,6 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt) opt = {}; if (argc == 1) return {st::bad_arguments, "No arguments specified. Use -h for help."}; - int c; optind = 0; while ((c = getopt_long(argc, argv, ":ho:i::yd:a:s:DS", getopt_options, NULL)) != -1) { @@ -69,34 +68,26 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt) break; case 'o': opt.path_out = optarg; - if (opt.path_out.empty()) { - fputs("output's file path cannot be empty\n", stderr); - return st::bad_arguments; - } + if (opt.path_out.empty()) + return {st::bad_arguments, "Output file path cannot be empty."}; break; case 'i': opt.inplace = optarg == nullptr ? ".otmp" : optarg; - if (strcmp(opt.inplace, "") == 0) { - fputs("the in-place suffix cannot be empty\n", stderr); - return st::bad_arguments; - } + if (strcmp(opt.inplace, "") == 0) + return {st::bad_arguments, "In-place suffix cannot be empty."}; break; case 'y': opt.overwrite = true; break; case 'd': - if (strchr(optarg, '=') != nullptr) { - fprintf(stderr, "invalid field name: '%s'\n", optarg); - return st::bad_arguments; - } + if (strchr(optarg, '=') != nullptr) + return {st::bad_arguments, "Invalid field name '"s + optarg + "'."}; opt.to_delete.emplace_back(optarg); break; case 'a': case 's': - if (strchr(optarg, '=') == NULL) { - fprintf(stderr, "invalid comment: '%s'\n", optarg); - return st::bad_arguments; - } + if (strchr(optarg, '=') == NULL) + return {st::bad_arguments, "Invalid comment '"s + optarg + "'."}; opt.to_add.emplace_back(optarg); if (c == 's') opt.to_delete.emplace_back(optarg); @@ -115,30 +106,20 @@ ot::status ot::parse_options(int argc, char** argv, ot::options& opt) (optopt ? "-"s + static_cast(optopt) : argv[optind - 1]) + "'."}; } } - if (opt.print_help) return st::ok; - if (optind != argc - 1) { - fputs("exactly one input file must be specified\n", stderr); - return st::bad_arguments; - } + if (optind != argc - 1) + return {st::bad_arguments, "Exactly one input file must be specified."}; opt.path_in = argv[optind]; - if (opt.path_in.empty()) { - fputs("input's file path cannot be empty\n", stderr); - return st::bad_arguments; - } - if (opt.inplace != nullptr && !opt.path_out.empty()) { - fputs("cannot combine --in-place and --output\n", stderr); - return st::bad_arguments; - } - if (opt.path_in == "-" && opt.set_all) { - fputs("can't open standard input for input when --set-all is specified\n", stderr); - return st::bad_arguments; - } - if (opt.path_in == "-" && opt.inplace) { - fputs("cannot modify standard input in-place\n", stderr); - return st::bad_arguments; - } + if (opt.path_in.empty()) + return {st::bad_arguments, "Input file path cannot be empty."}; + if (opt.inplace != nullptr && !opt.path_out.empty()) + return {st::bad_arguments, "Cannot combine --in-place and --output."}; + if (opt.path_in == "-" && opt.set_all) + return {st::bad_arguments, + "Cannot use standard input as input file when --set-all is specified."}; + if (opt.path_in == "-" && opt.inplace) + return {st::bad_arguments, "Cannot modify standard input in-place."}; return st::ok; } diff --git a/src/opustags.h b/src/opustags.h index b11a7e5..9da97bd 100644 --- a/src/opustags.h +++ b/src/opustags.h @@ -428,8 +428,6 @@ struct options { * consistency of its arguments. * * On error, the state of the options structure is unspecified. - * - * \todo Return error messages in the status instead of writing them to stderr. */ status parse_options(int argc, char** argv, options& opt); diff --git a/t/cli.cc b/t/cli.cc index 7ccfc57..ccccbf2 100644 --- a/t/cli.cc +++ b/t/cli.cc @@ -18,28 +18,40 @@ void check_read_comments() throw failure("parsed user comments did not match expectations"); } -void check_parse_options() +void check_bad_arguments() { auto error_case = [](std::vector args, const char* message, const std::string& name) { ot::options opt; - ot::status rc = parse_options(args.size(), const_cast(args.data()), opt); - if (rc != ot::st::bad_arguments) - throw failure("bad error code when parsing " + name); + ot::status rc = ot::parse_options(args.size(), const_cast(args.data()), opt); + if (rc.code != ot::st::bad_arguments) + throw failure("bad error code for case " + name); if (rc.message != message) - throw failure("bad error message when parsing " + name); + throw failure("bad error message for case " + name + ", got: " + rc.message); }; + error_case({"opustags"}, "No arguments specified. Use -h for help.", "no arguments"); + error_case({"opustags", "--output", ""}, "Output file path cannot be empty.", "empty output path"); + error_case({"opustags", "--in-place="}, "In-place suffix cannot be empty.", "empty in-place suffix"); + error_case({"opustags", "--delete", "X="}, "Invalid field name 'X='.", "bad field name for -d"); + error_case({"opustags", "-a", "X"}, "Invalid comment 'X'.", "bad comment for -a"); + error_case({"opustags", "--set", "X"}, "Invalid comment 'X'.", "bad comment for --set"); error_case({"opustags", "-a"}, "Missing value for option '-a'.", "short option with missing value"); error_case({"opustags", "--add"}, "Missing value for option '--add'.", "long option with missing value"); error_case({"opustags", "-x"}, "Unrecognized option '-x'.", "unrecognized short option"); error_case({"opustags", "--derp"}, "Unrecognized option '--derp'.", "unrecognized long option"); error_case({"opustags", "-x=y"}, "Unrecognized option '-x'.", "unrecognized short option with value"); error_case({"opustags", "--derp=y"}, "Unrecognized option '--derp=y'.", "unrecognized long option with value"); + error_case({"opustags", "-aX=Y"}, "Exactly one input file must be specified.", "no input file"); + error_case({"opustags", ""}, "Input file path cannot be empty.", "empty input file path"); + error_case({"opustags", "-i", "-o", "/dev/null", "-"}, "Cannot combine --in-place and --output.", "in-place + output"); + error_case({"opustags", "-S", "-"}, "Cannot use standard input as input file when --set-all is specified.", + "set all and read opus from stdin"); + error_case({"opustags", "-i", "-"}, "Cannot modify standard input in-place.", "write stdin in-place"); } int main(int argc, char **argv) { std::cout << "1..2\n"; run(check_read_comments, "check tags parsing"); - run(check_parse_options, "check options parsing"); + run(check_bad_arguments, "check options parsing errors"); return 0; } diff --git a/t/opustags.t b/t/opustags.t index 9b8e0a7..b11ac9f 100644 --- a/t/opustags.t +++ b/t/opustags.t @@ -133,12 +133,12 @@ EOF is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'the file did not change'); is_deeply(opustags(qw(-i out.opus -a fatal=yes -a FOO -a BAR)), ['', <<'EOF', 256], 'bad tag with --add'); -invalid comment: 'FOO' +error: Invalid comment 'FOO'. EOF is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'the file did not change'); is_deeply(opustags(qw(-i out.opus -s fatal=yes -s FOO -s BAR)), ['', <<'EOF', 256], 'bad tag with --set'); -invalid comment: 'FOO' +error: Invalid comment 'FOO'. EOF is(md5('out.opus'), '66780307a6081523dc9040f3c47b0448', 'the file did not change');