Skip to content

Commit

Permalink
Make unlimited arguments less grabby
Browse files Browse the repository at this point in the history
  • Loading branch information
@henryiii
henryiii committed Nov 22, 2017
1 parent 4a3b57a commit bc2e427
Show file tree
Hide file tree
Showing 4 changed files with 230 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
64 changes: 51 additions & 13 deletions include/CLI/App.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(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<int>(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_)
Expand Down Expand Up @@ -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<int>(opt->count()) < opt->get_expected())
retval = static_cast<size_t>(opt->get_expected()) - opt->count();

Expand Down Expand Up @@ -1323,7 +1326,7 @@ class App {
///
/// Unlike the others, this one will always allow fallthrough
void _parse_subcommand(std::vector<std::string> &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())) {
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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;
}
};
Expand Down
172 changes: 172 additions & 0 deletions tests/AppTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> 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<std::string>({"one", "two"}));
}

TEST_F(TApp, RequiredOptsDoubleShort) {

std::vector<std::string> 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<std::string>({"one", "two"}));
}

TEST_F(TApp, RequiredOptsUnlimited) {

std::vector<std::string> 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<std::string>({"one", "two"}));

app.reset();
args = {"--str", "one", "two"};
run();
EXPECT_EQ(strs, std::vector<std::string>({"one", "two"}));

app.reset();
app.allow_extras();
run();
EXPECT_EQ(strs, std::vector<std::string>({"one"}));
EXPECT_EQ(app.remaining(), std::vector<std::string>({"two"}));

app.reset();
app.allow_extras(false);
std::vector<std::string> remain;
app.add_option("positional", remain);
run();
EXPECT_EQ(strs, std::vector<std::string>({"one"}));
EXPECT_EQ(remain, std::vector<std::string>({"two"}));
}

TEST_F(TApp, RequiredOptsUnlimitedShort) {

std::vector<std::string> 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<std::string>({"one", "two"}));

app.reset();
args = {"-s", "one", "two"};
run();
EXPECT_EQ(strs, std::vector<std::string>({"one", "two"}));

app.reset();
app.allow_extras();
run();
EXPECT_EQ(strs, std::vector<std::string>({"one"}));
EXPECT_EQ(app.remaining(), std::vector<std::string>({"two"}));

app.reset();
app.allow_extras(false);
std::vector<std::string> remain;
app.add_option("positional", remain);
run();
EXPECT_EQ(strs, std::vector<std::string>({"one"}));
EXPECT_EQ(remain, std::vector<std::string>({"two"}));
}

TEST_F(TApp, RequireOptPriority) {

std::vector<std::string> strs;
app.add_option("--str", strs)->required();
std::vector<std::string> remain;
app.add_option("positional", remain)->expected(2);

args = {"--str", "one", "two", "three"};
run();

EXPECT_EQ(strs, std::vector<std::string>({"one"}));
EXPECT_EQ(remain, std::vector<std::string>({"two", "three"}));

app.reset();
args = {"two", "three", "--str", "one", "four"};
run();

EXPECT_EQ(strs, std::vector<std::string>({"one", "four"}));
EXPECT_EQ(remain, std::vector<std::string>({"two", "three"}));
}

TEST_F(TApp, RequireOptPriorityShort) {

std::vector<std::string> strs;
app.add_option("-s", strs)->required();
std::vector<std::string> remain;
app.add_option("positional", remain)->expected(2);

args = {"-s", "one", "two", "three"};
run();

EXPECT_EQ(strs, std::vector<std::string>({"one"}));
EXPECT_EQ(remain, std::vector<std::string>({"two", "three"}));

app.reset();
args = {"two", "three", "-s", "one", "four"};
run();

EXPECT_EQ(strs, std::vector<std::string>({"one", "four"}));
EXPECT_EQ(remain, std::vector<std::string>({"two", "three"}));
}

TEST_F(TApp, NotRequiedExpectedDouble) {

std::vector<std::string> 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;
Expand Down
6 changes: 6 additions & 0 deletions tests/SubcommandTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit bc2e427

Please sign in to comment.