From cb906d1aabfa44f97f035950d8f5cd50e754510d Mon Sep 17 00:00:00 2001 From: Nathan Hourt Date: Tue, 22 Aug 2017 14:18:30 -0500 Subject: [PATCH 1/6] Fix #23: Respect fallthrough_ in _valid_subcommand _valid_subcommand checks whether its argument appears to be a valid subcommand name or not; however, if it doesn't recognize the name, it always checks if its parent does. As described in in issue #23, this can cause incorrect behavior. To avoid this, check if fallthrough is disabled first, and do not consult the parent's known subcommands if fallthrough is disabled. --- include/CLI/App.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/CLI/App.hpp b/include/CLI/App.hpp index 02863cf1..54f4e5c8 100644 --- a/include/CLI/App.hpp +++ b/include/CLI/App.hpp @@ -857,7 +857,7 @@ class App { for(const App_p &com : subcommands_) if(com->check_name(current)) return true; - if(parent_ != nullptr) + if(parent_ != nullptr && fallthrough_) return parent_->_valid_subcommand(current); else return false; From cef5dfa58d6c4e365229d378db6f34603be3215e Mon Sep 17 00:00:00 2001 From: Henry Fredrick Schreiner Date: Tue, 22 Aug 2017 21:55:34 -0700 Subject: [PATCH 2/6] Revert change for now, add helper function --- include/CLI/App.hpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/include/CLI/App.hpp b/include/CLI/App.hpp index 54f4e5c8..082a4f80 100644 --- a/include/CLI/App.hpp +++ b/include/CLI/App.hpp @@ -1064,6 +1064,19 @@ class App { } } + /// Count the required remaining positional arguments + size_t _count_remaining_required_positionals() const { + size_t retval = 0; + for(const Option_p &opt : options_) + if(opt->get_positional() + && opt->get_required() + && opt->get_expected() > 0 + && static_cast(opt->count()) < opt->get_expected()) + retval = static_cast(opt->get_expected()) - opt->count(); + + return retval; + } + /// Parse a positional, go up the tree to check void _parse_positional(std::vector &args) { @@ -1080,7 +1093,7 @@ class App { } } - if(parent_ != nullptr && fallthrough_) + if(parent_ != nullptr)// TODO && fallthrough_) return parent_->_parse_positional(args); else { args.pop_back(); From b480e2f1636539b25b4f56eaf16992be3509fd82 Mon Sep 17 00:00:00 2001 From: Henry Fredrick Schreiner Date: Tue, 22 Aug 2017 22:35:37 -0700 Subject: [PATCH 3/6] Adding fix for #23; throws ExtraError instead of RequiredError?... --- include/CLI/App.hpp | 6 +++-- tests/SubcommandTest.cpp | 49 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/include/CLI/App.hpp b/include/CLI/App.hpp index 082a4f80..c2f9d3ca 100644 --- a/include/CLI/App.hpp +++ b/include/CLI/App.hpp @@ -857,7 +857,7 @@ class App { for(const App_p &com : subcommands_) if(com->check_name(current)) return true; - if(parent_ != nullptr && fallthrough_) + if(parent_ != nullptr) return parent_->_valid_subcommand(current); else return false; @@ -1093,7 +1093,7 @@ class App { } } - if(parent_ != nullptr)// TODO && fallthrough_) + if(parent_ != nullptr && fallthrough_) return parent_->_parse_positional(args); else { args.pop_back(); @@ -1113,6 +1113,8 @@ class App { /// /// Unlike the others, this one will always allow fallthrough void _parse_subcommand(std::vector &args) { + if(_count_remaining_required_positionals() > 0) + return _parse_positional(args); for(const App_p &com : subcommands_) { if(com->check_name(args.back())) { args.pop_back(); diff --git a/tests/SubcommandTest.cpp b/tests/SubcommandTest.cpp index c2753705..46372b80 100644 --- a/tests/SubcommandTest.cpp +++ b/tests/SubcommandTest.cpp @@ -74,6 +74,55 @@ TEST_F(TApp, MultiSubFallthrough) { EXPECT_THROW(app.got_subcommand("sub3"), CLI::OptionNotFound); } +TEST_F(TApp, RequiredAndSubcoms) { // #23 + + std::string baz; + app.add_option("baz", baz, "Baz Description", true)->required(); + auto foo = app.add_subcommand("foo"); + auto bar = app.add_subcommand("bar"); + + args = {"bar", "foo"}; + EXPECT_NO_THROW(run()); + EXPECT_TRUE(*foo); + EXPECT_FALSE(*bar); + EXPECT_EQ(baz, "bar"); + + app.reset(); + args = {"foo"}; + EXPECT_NO_THROW(run()); + EXPECT_FALSE(*foo); + EXPECT_EQ(baz, "foo"); + + app.reset(); + args = {"foo", "foo"}; + EXPECT_NO_THROW(run()); + EXPECT_TRUE(*foo); + EXPECT_EQ(baz, "foo"); + + app.reset(); + args = {"foo", "other"}; + EXPECT_THROW(run(), CLI::ParseError); +} + +TEST_F(TApp, RequiredAndSubcomFallthrough) { + + std::string baz; + app.add_option("baz", baz, "Baz Description", true)->required(); + app.add_subcommand("foo"); + auto bar = app.add_subcommand("bar"); + app.fallthrough(); + + args = {"other", "bar"}; + run(); + EXPECT_TRUE(bar); + EXPECT_EQ(baz, "other"); + + app.reset(); + args = {"bar", "other2"}; + EXPECT_THROW(run(), CLI::ParseError); // RequiredError or ExtrasError (actual) + +} + TEST_F(TApp, Callbacks) { auto sub1 = app.add_subcommand("sub1"); sub1->set_callback([]() { throw CLI::Success(); }); From de56a9c87a81fffa821c229c4fd4b1bb75db2b07 Mon Sep 17 00:00:00 2001 From: Henry Fredrick Schreiner Date: Wed, 23 Aug 2017 09:41:45 -0700 Subject: [PATCH 4/6] Fixing the foo foo problem --- include/CLI/App.hpp | 2 +- tests/SubcommandTest.cpp | 32 +++++++++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/include/CLI/App.hpp b/include/CLI/App.hpp index c2f9d3ca..9ab4cd6a 100644 --- a/include/CLI/App.hpp +++ b/include/CLI/App.hpp @@ -855,7 +855,7 @@ class App { bool _valid_subcommand(const std::string ¤t) const { for(const App_p &com : subcommands_) - if(com->check_name(current)) + if(com->check_name(current) && !*com) return true; if(parent_ != nullptr) return parent_->_valid_subcommand(current); diff --git a/tests/SubcommandTest.cpp b/tests/SubcommandTest.cpp index 46372b80..127a3e80 100644 --- a/tests/SubcommandTest.cpp +++ b/tests/SubcommandTest.cpp @@ -101,13 +101,13 @@ TEST_F(TApp, RequiredAndSubcoms) { // #23 app.reset(); args = {"foo", "other"}; - EXPECT_THROW(run(), CLI::ParseError); + EXPECT_THROW(run(), CLI::ExtrasError); // RequiredError } TEST_F(TApp, RequiredAndSubcomFallthrough) { std::string baz; - app.add_option("baz", baz, "Baz Description", true)->required(); + app.add_option("baz", baz)->required(); app.add_subcommand("foo"); auto bar = app.add_subcommand("bar"); app.fallthrough(); @@ -119,8 +119,34 @@ TEST_F(TApp, RequiredAndSubcomFallthrough) { app.reset(); args = {"bar", "other2"}; - EXPECT_THROW(run(), CLI::ParseError); // RequiredError or ExtrasError (actual) + EXPECT_THROW(run(), CLI::ExtrasError); // RequiredError +} +TEST_F(TApp, FooFooProblem) { + + std::string baz_str, other_str; + auto baz = app.add_option("baz", baz_str); + auto foo = app.add_subcommand("foo"); + auto other = foo->add_option("other", other_str); + + args = {"foo", "foo"}; + run(); + EXPECT_TRUE(*foo); + EXPECT_FALSE(*baz); + EXPECT_TRUE(*other); // Fails + EXPECT_EQ(baz_str, ""); + EXPECT_EQ(other_str, "foo"); // Fails + + app.reset(); + baz_str = ""; + other_str = ""; + baz->required(); + run(); + EXPECT_TRUE(*foo); + EXPECT_TRUE(*baz); + EXPECT_FALSE(*other); + EXPECT_EQ(baz_str, "foo"); + EXPECT_EQ(other_str, ""); } TEST_F(TApp, Callbacks) { From 4dcab42bc77205b9f2bb7d6e13b4021ac09a41d5 Mon Sep 17 00:00:00 2001 From: Henry Fredrick Schreiner Date: Wed, 23 Aug 2017 09:59:55 -0700 Subject: [PATCH 5/6] Convincing myself that the errors are correct --- tests/SubcommandTest.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/SubcommandTest.cpp b/tests/SubcommandTest.cpp index 127a3e80..21c26c28 100644 --- a/tests/SubcommandTest.cpp +++ b/tests/SubcommandTest.cpp @@ -101,7 +101,7 @@ TEST_F(TApp, RequiredAndSubcoms) { // #23 app.reset(); args = {"foo", "other"}; - EXPECT_THROW(run(), CLI::ExtrasError); // RequiredError + EXPECT_THROW(run(), CLI::ExtrasError); } TEST_F(TApp, RequiredAndSubcomFallthrough) { @@ -119,7 +119,7 @@ TEST_F(TApp, RequiredAndSubcomFallthrough) { app.reset(); args = {"bar", "other2"}; - EXPECT_THROW(run(), CLI::ExtrasError); // RequiredError + EXPECT_THROW(run(), CLI::ExtrasError); } TEST_F(TApp, FooFooProblem) { @@ -133,9 +133,9 @@ TEST_F(TApp, FooFooProblem) { run(); EXPECT_TRUE(*foo); EXPECT_FALSE(*baz); - EXPECT_TRUE(*other); // Fails + EXPECT_TRUE(*other); EXPECT_EQ(baz_str, ""); - EXPECT_EQ(other_str, "foo"); // Fails + EXPECT_EQ(other_str, "foo"); app.reset(); baz_str = ""; From bd87bdff53e68e5e29d6112bc164d5c1dde40cee Mon Sep 17 00:00:00 2001 From: Henry Fredrick Schreiner Date: Wed, 23 Aug 2017 10:44:03 -0700 Subject: [PATCH 6/6] Docs update for subcom/positional --- CHANGELOG.md | 4 ++++ README.md | 2 ++ 2 files changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f81c33e..9c8c41c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## Version 1.2 (in progress) + +* Required positionals now take priority over subcommands [#23](https://github.com/CLIUtils/CLI11/issues/23) + ## Version 1.1 * Added simple support for enumerations, allow non-printable objects [#12](https://github.com/CLIUtils/CLI11/issues/12) diff --git a/README.md b/README.md index 66317ae2..df4c0651 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,8 @@ There are several options that are supported on the main app and subcommands. Th * `.allow_extras()`: Do not throw an error if extra arguments are left over (Only useful on the main `App`, as that's the one that throws errors). * `.prefix_command()`: Like `allow_extras`, but stop immediately on the first unrecognised item. It is ideal for allowing your app to be a "prefix" to calling another app. +> Note: if you have a fixed number of required positional options, that will match before subcommand names. + ## Configuration file ```cpp