diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c0e29b3..41b057f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ * Failure messages are now customizable, with a shorter default [#52](https://github.com/CLIUtils/CLI11/pull/52) * `require_subcommand` now offers a two-argument form and negative values on the one-argument form are more useful [#51](https://github.com/CLIUtils/CLI11/pull/51) * Subcommands no longer match after the max required number is obtained [#51](https://github.com/CLIUtils/CLI11/pull/51) +* Unlimited options no longer prioritize over extras or remaining/unlimited positionals [#51](https://github.com/CLIUtils/CLI11/pull/51) ## Version 1.2 diff --git a/include/CLI/App.hpp b/include/CLI/App.hpp index 70c5d5a3..48ad02da 100644 --- a/include/CLI/App.hpp +++ b/include/CLI/App.hpp @@ -1156,14 +1156,17 @@ class App { // Verify required options for(const Option_p &opt : options_) { - // Required - if(opt->get_required()) { - if(opt->count() == 0) { + // Required or partially filled + if(opt->get_required() || opt->count() != 0) { + + // Required but empty + if(opt->get_required() && opt->count() == 0) throw RequiredError(opt->help_name() + " is required"); - } else if(static_cast(opt->count()) < opt->get_expected()) { - throw RequiredError(opt->help_name() + " required at least " + std::to_string(opt->get_expected()) + + + // Partially filled + if(opt->get_expected() > 0 && static_cast(opt->count()) < opt->get_expected()) + throw RequiredError(opt->help_name() + " requires " + std::to_string(opt->get_expected()) + " arguments"); - } } // Requires for(const Option *opt_req : opt->requires_) @@ -1278,10 +1281,10 @@ class App { } /// Count the required remaining positional arguments - size_t _count_remaining_required_positionals() const { + size_t _count_remaining_positionals(bool required = false) const { size_t retval = 0; for(const Option_p &opt : options_) - if(opt->get_positional() && opt->get_required() && opt->get_expected() > 0 && + if(opt->get_positional() && (!required || opt->get_required()) && opt->get_expected() > 0 && static_cast(opt->count()) < opt->get_expected()) retval = static_cast(opt->get_expected()) - opt->count(); @@ -1323,7 +1326,7 @@ class App { /// /// Unlike the others, this one will always allow fallthrough void _parse_subcommand(std::vector &args) { - if(_count_remaining_required_positionals() > 0) + if(_count_remaining_positionals(/* required */ true) > 0) return _parse_positional(args); for(const App_p &com : subcommands_) { if(com->check_name(args.back())) { @@ -1384,11 +1387,29 @@ class App { rest = ""; } + // Unlimited vector parser if(num == -1) { + bool already_ate_one = false; // Make sure we always eat one while(!args.empty() && _recognize(args.back()) == detail::Classifer::NONE) { + if(already_ate_one) { + // If allow extras is true, don't keep eating + if(get_allow_extras()) + break; + + // If any positionals remain, don't keep eating + if(_count_remaining_positionals() > 0) + break; + + // If there are any unlimited positionals, those also take priority + if(std::any_of(std::begin(options_), std::end(options_), [](const Option_p &opt) { + return opt->get_positional() && opt->get_expected() < 0; + })) + break; + } op->add_result(args.back()); parse_order_.push_back(op.get()); args.pop_back(); + already_ate_one = true; } } else while(num > 0 && !args.empty()) { @@ -1445,21 +1466,38 @@ class App { } else if(num == 0) { op->add_result(""); parse_order_.push_back(op.get()); - } - - if(num == -1) { + } else if(num == -1) { + // Unlimited vector parser + bool already_ate_one = false; // Make sure we always eat one while(!args.empty() && _recognize(args.back()) == detail::Classifer::NONE) { + if(already_ate_one) { + // If allow extras is true, don't keep eating + if(get_allow_extras()) + break; + + // If any positionals remain, don't keep eating + if(_count_remaining_positionals() > 0) + break; + + // If there are any unlimited positionals, those also take priority + if(std::any_of(std::begin(options_), std::end(options_), [](const Option_p &opt) { + return opt->get_positional() && opt->get_expected() < 0; + })) + break; + } op->add_result(args.back()); parse_order_.push_back(op.get()); args.pop_back(); + already_ate_one = true; } - } else + } else { while(num > 0 && !args.empty()) { num--; op->add_result(args.back()); parse_order_.push_back(op.get()); args.pop_back(); } + } return; } }; diff --git a/tests/AppTest.cpp b/tests/AppTest.cpp index 8bfa0f15..ba86ca87 100644 --- a/tests/AppTest.cpp +++ b/tests/AppTest.cpp @@ -230,6 +230,178 @@ TEST_F(TApp, TakeLastOpt) { EXPECT_EQ(str, "two"); } +TEST_F(TApp, RequiredOptsSingle) { + + std::string str; + app.add_option("--str", str)->required(); + + args = {"--str"}; + + EXPECT_THROW(run(), CLI::RequiredError); +} + +TEST_F(TApp, RequiredOptsSingleShort) { + + std::string str; + app.add_option("-s", str)->required(); + + args = {"-s"}; + + EXPECT_THROW(run(), CLI::RequiredError); +} + +TEST_F(TApp, RequiredOptsDouble) { + + std::vector strs; + app.add_option("--str", strs)->required()->expected(2); + + args = {"--str", "one"}; + + EXPECT_THROW(run(), CLI::RequiredError); + + app.reset(); + args = {"--str", "one", "two"}; + + run(); + + EXPECT_EQ(strs, std::vector({"one", "two"})); +} + +TEST_F(TApp, RequiredOptsDoubleShort) { + + std::vector strs; + app.add_option("-s", strs)->required()->expected(2); + + args = {"-s", "one"}; + + EXPECT_THROW(run(), CLI::RequiredError); + + app.reset(); + args = {"-s", "one", "two"}; + + run(); + + EXPECT_EQ(strs, std::vector({"one", "two"})); +} + +TEST_F(TApp, RequiredOptsUnlimited) { + + std::vector strs; + app.add_option("--str", strs)->required(); + + args = {"--str"}; + EXPECT_THROW(run(), CLI::RequiredError); + + app.reset(); + args = {"--str", "one", "--str", "two"}; + run(); + EXPECT_EQ(strs, std::vector({"one", "two"})); + + app.reset(); + args = {"--str", "one", "two"}; + run(); + EXPECT_EQ(strs, std::vector({"one", "two"})); + + app.reset(); + app.allow_extras(); + run(); + EXPECT_EQ(strs, std::vector({"one"})); + EXPECT_EQ(app.remaining(), std::vector({"two"})); + + app.reset(); + app.allow_extras(false); + std::vector remain; + app.add_option("positional", remain); + run(); + EXPECT_EQ(strs, std::vector({"one"})); + EXPECT_EQ(remain, std::vector({"two"})); +} + +TEST_F(TApp, RequiredOptsUnlimitedShort) { + + std::vector strs; + app.add_option("-s", strs)->required(); + + args = {"-s"}; + EXPECT_THROW(run(), CLI::RequiredError); + + app.reset(); + args = {"-s", "one", "-s", "two"}; + run(); + EXPECT_EQ(strs, std::vector({"one", "two"})); + + app.reset(); + args = {"-s", "one", "two"}; + run(); + EXPECT_EQ(strs, std::vector({"one", "two"})); + + app.reset(); + app.allow_extras(); + run(); + EXPECT_EQ(strs, std::vector({"one"})); + EXPECT_EQ(app.remaining(), std::vector({"two"})); + + app.reset(); + app.allow_extras(false); + std::vector remain; + app.add_option("positional", remain); + run(); + EXPECT_EQ(strs, std::vector({"one"})); + EXPECT_EQ(remain, std::vector({"two"})); +} + +TEST_F(TApp, RequireOptPriority) { + + std::vector strs; + app.add_option("--str", strs)->required(); + std::vector remain; + app.add_option("positional", remain)->expected(2); + + args = {"--str", "one", "two", "three"}; + run(); + + EXPECT_EQ(strs, std::vector({"one"})); + EXPECT_EQ(remain, std::vector({"two", "three"})); + + app.reset(); + args = {"two", "three", "--str", "one", "four"}; + run(); + + EXPECT_EQ(strs, std::vector({"one", "four"})); + EXPECT_EQ(remain, std::vector({"two", "three"})); +} + +TEST_F(TApp, RequireOptPriorityShort) { + + std::vector strs; + app.add_option("-s", strs)->required(); + std::vector remain; + app.add_option("positional", remain)->expected(2); + + args = {"-s", "one", "two", "three"}; + run(); + + EXPECT_EQ(strs, std::vector({"one"})); + EXPECT_EQ(remain, std::vector({"two", "three"})); + + app.reset(); + args = {"two", "three", "-s", "one", "four"}; + run(); + + EXPECT_EQ(strs, std::vector({"one", "four"})); + EXPECT_EQ(remain, std::vector({"two", "three"})); +} + +TEST_F(TApp, NotRequiedExpectedDouble) { + + std::vector strs; + app.add_option("--str", strs)->expected(2); + + args = {"--str", "one"}; + + EXPECT_THROW(run(), CLI::RequiredError); +} + TEST_F(TApp, EnumTest) { enum Level : std::int32_t { High, Medium, Low }; Level level = Level::Low; diff --git a/tests/SubcommandTest.cpp b/tests/SubcommandTest.cpp index 9b83eb95..96bad491 100644 --- a/tests/SubcommandTest.cpp +++ b/tests/SubcommandTest.cpp @@ -677,6 +677,12 @@ TEST_F(ManySubcommands, Required2Exact) { EXPECT_EQ(sub2->remaining(), vs_t({"sub3"})); } +TEST_F(ManySubcommands, Required4Failure) { + app.require_subcommand(4); + + EXPECT_THROW(run(), CLI::RequiredError); +} + TEST_F(ManySubcommands, Required1Fuzzy) { app.require_subcommand(0, 1);