add a test of duplicate named subcommands in different option_groups and make su…

…re that executes them over running the same one twice. (#247) make duplicate subcommands work

add a test of duplicate named subcommands in different option_groups and make su…
…re that executes them over running the same one twice. (#247) make duplicate subcommands work
Philip Top · Henry Schreiner
1 parent b8ebce75
Showing 4 changed files with 87 additions and 20 deletions
README.md
include/CLI/App.hpp
tests/OptionGroupTest.cpp
tests/SubcommandTest.cpp
@@ -283,7 +283,7 @@ Before parsing, you can set the following options:
 -   `->envname(name)`: Gets the value from the environment if present and not passed on the command line.
 -   `->group(name)`: The help group to put the option in. No effect for positional options. Defaults to `"Options"`. `""` will not show up in the help print (hidden).
 -   `->ignore_case()`: Ignore the case on the command line (also works on subcommands, does not affect arguments).
--   `->ignore_underscore()`: 🚧 Ignore any underscores in the options names (also works on subcommands, does not affect arguments). For example "option_one" will match with "optionone".  This does not apply to short form options since they only have one character
+-   `->ignore_underscore()`: 🆕 Ignore any underscores in the options names (also works on subcommands, does not affect arguments). For example "option_one" will match with "optionone".  This does not apply to short form options since they only have one character
 -   `->disable_flag_override()`: 🚧 from the command line long form flag option can be assigned a value on the command line using the `=` notation `--flag=value`. If this behavior is not desired, the `disable_flag_override()` disables it and will generate an exception if it is done on the command line.  The `=` does not work with short form flag options.
 -    `->delimiter(char)`: 🚧 allows specification of a custom delimiter for separating single arguments into vector arguments, for example specifying `->delimiter(',')` on an option would result in `--opt=1,2,3` producing 3 elements of a vector and the equivalent of --opt 1 2 3 assuming opt is a vector value
 -   `->description(str)`: 🆕 Set/change the description.
@@ -379,7 +379,7 @@ of `IsMember`:
      *   `auto p = std::make_shared<std::vector<std::string>>(std::initializer_list<std::string>("one", "two")); CLI::IsMember(p)`: You can modify `p` later.
 * 🚧 The `Transformer` and `CheckedTransformer` Validators transform one value into another. Any container or copyable pointer (including `std::shared_ptr`) to a container that generates pairs of values can be passed to these `Validator's`; the container just needs to be iterable and have a `::value_type` that consists of pairs. The key type should be convertible from a string, and the value type should be convertible to a string  You can use an initializer list directly if you like. If you need to modify the map later, the pointer form lets you do that; the description message will correctly refer to the current version of the map.  `Transformer` does not do any checking so values not in the map are ignored.  `CheckedTransformer` takes an extra step of verifying that the value is either one of the map key values, or one of the expected output values, and if not will generate a ValidationError.  A Transformer placed using `check` will not do anything.  
 After specifying a map of options, you can also specify "filter" just like in CLI::IsMember.
-Here are some examples (`Transfomer` and `CheckedTransformer` are interchangeable in the examples)
+Here are some examples (`Transformer` and `CheckedTransformer` are interchangeable in the examples)
 of `Transformer`:
  
      *   `CLI::Transformer({{"key1", "map1"},{"key2","map2"}})`: Select from key values and produce map values.
@@ -453,7 +453,7 @@ All `App`s have a `get_subcommands()` method, which returns a list of pointers t
 For many cases, however, using an app's callback may be easier. Every app executes a callback function after it parses; just use a lambda function (with capture to get parsed values) to `.callback`. If you throw `CLI::Success` or `CLI::RuntimeError(return_value)`, you can
 even exit the program through the callback. The main `App` has a callback slot, as well, but it is generally not as useful.
 You are allowed to throw `CLI::Success` in the callbacks.
-Multiple subcommands are allowed, to allow [`Click`][click] like series of commands (order is preserved).
+Multiple subcommands are allowed, to allow [`Click`][click] like series of commands (order is preserved).  The same subcommand can be triggered multiple times but all positional arguments will take precedence over the second and future calls of the subcommand.  `->count()` on the subcommand will return the number of times the subcommand was called.  The subcommand callback will only be triggered once.  
  
 🚧 Subcommands may also have an empty name either by calling `add_subcommand` with an empty string for the name or with no arguments.
 Nameless subcommands function a similarly to groups in the main `App`. See [Option groups](#option-groups) to see how this might work.  If an option is not defined in the main App, all nameless subcommands are checked as well.  This allows for the options to be defined in a composable group.  The `add_subcommand` function has an overload for adding a `shared_ptr<App>` so the subcommand(s) could be defined in different components and merged into a main `App`, or possibly multiple `Apps`.  Multiple nameless subcommands are allowed.
@@ -1045,7 +1045,7 @@ class App {
  
     /// Check to see if a subcommand is part of this command (text version)
     App *get_subcommand(std::string subcom) const {
-        auto subc = _find_subcommand(subcom, false);
+        auto subc = _find_subcommand(subcom, false, false);
         if(subc == nullptr)
             throw OptionNotFound(subcom);
         return subc;
@@ -1572,7 +1572,7 @@ class App {
     /// Get the status of required
     bool get_required() const { return required_; }
  
-    /// Get the status of required
+    /// Get the status of disabled
     bool get_disabled() const { return disabled_; }
  
     /// Get the status of allow extras
@@ -1736,8 +1736,8 @@ class App {
         if(require_subcommand_max_ != 0 && parsed_subcommands_.size() >= require_subcommand_max_) {
             return parent_ != nullptr && parent_->_valid_subcommand(current);
         }
-        auto com = _find_subcommand(current, true);
-        if((com != nullptr) && !*com) {
+        auto com = _find_subcommand(current, true, true);
+        if(com != nullptr) {
             return true;
         }
         // Check parent if exists, else return false
@@ -2135,33 +2135,49 @@ class App {
         if(parent_ != nullptr && fallthrough_)
             return parent_->_parse_positional(args);
         else {
-            if(positionals_at_end_) {
-                throw CLI::ExtrasError(args);
+            /// now try one last gasp at subcommands that have been executed before, go to root app and try to find a
+            /// subcommand in a broader way
+            auto parent_app = this;
+            while(parent_app->parent_ != nullptr) {
+                parent_app = parent_app->parent_;
             }
-            args.pop_back();
-            missing_.emplace_back(detail::Classifier::NONE, positional);
+            auto com = parent_app->_find_subcommand(args.back(), true, false);
+            if((com != nullptr) &&
+               ((com->parent_->require_subcommand_max_ == 0) ||
+                (com->parent_->require_subcommand_max_ > com->parent_->parsed_subcommands_.size()))) {
+                args.pop_back();
+                com->_parse(args);
+            } else {
+                if(positionals_at_end_) {
+                    throw CLI::ExtrasError(args);
+                }
+                args.pop_back();
+                missing_.emplace_back(detail::Classifier::NONE, positional);
  
-            if(prefix_command_) {
-                while(!args.empty()) {
-                    missing_.emplace_back(detail::Classifier::NONE, args.back());
-                    args.pop_back();
+                if(prefix_command_) {
+                    while(!args.empty()) {
+                        missing_.emplace_back(detail::Classifier::NONE, args.back());
+                        args.pop_back();
+                    }
                 }
             }
         }
     }
  
-    /// Locate a subcommand by name
-    App *_find_subcommand(const std::string &subc_name, bool ignore_disabled) const noexcept {
+    /// Locate a subcommand by name with two conditions, should disabled subcommands be ignored, and should used
+    /// subcommands be ignored
+    App *_find_subcommand(const std::string &subc_name, bool ignore_disabled, bool ignore_used) const noexcept {
         for(const App_p &com : subcommands_) {
             if((com->disabled_) && (ignore_disabled))
                 continue;
             if(com->get_name().empty()) {
-                auto subc = com->_find_subcommand(subc_name, ignore_disabled);
+                auto subc = com->_find_subcommand(subc_name, ignore_disabled, ignore_used);
                 if(subc != nullptr) {
                     return subc;
                 }
             } else if(com->check_name(subc_name)) {
-                return com.get();
+                if((!*com) || (!ignore_used))
+                    return com.get();
             }
         }
         return nullptr;
@@ -2173,7 +2189,7 @@ class App {
     void _parse_subcommand(std::vector<std::string> &args) {
         if(_count_remaining_positionals(/* required */ true) > 0)
             return _parse_positional(args);
-        auto com = _find_subcommand(args.back(), true);
+        auto com = _find_subcommand(args.back(), true, true);
         if(com != nullptr) {
             args.pop_back();
             if(std::find(std::begin(parsed_subcommands_), std::end(parsed_subcommands_), com) ==
@@ -501,3 +501,39 @@ TEST_F(ManyGroups, DisableFirst) {
     args = {"--name1", "test", "--name2", "test3", "--name3=test3"};
     EXPECT_NO_THROW(run());
 }
+
+TEST_F(ManyGroups, SameSubcommand) {
+    // only 1 group can be used
+    remove_required();
+    auto sub1 = g1->add_subcommand("sub1");
+    auto sub2 = g2->add_subcommand("sub1");
+    auto sub3 = g3->add_subcommand("sub1");
+
+    args = {"sub1", "sub1", "sub1"};
+
+    run();
+
+    EXPECT_TRUE(*sub1);
+    EXPECT_TRUE(*sub2);
+    EXPECT_TRUE(*sub3);
+    /// This should be made to work at some point
+    /*auto subs = app.get_subcommands();
+    EXPECT_EQ(subs.size(), 3u);
+    EXPECT_EQ(subs[0], sub1);
+    EXPECT_EQ(subs[1], sub2);
+    EXPECT_EQ(subs[2], sub3);
+    */
+    args = {"sub1", "sub1", "sub1", "sub1"};
+    // for the 4th and future ones they will route to the first one
+    run();
+    EXPECT_EQ(sub1->count(), 2u);
+    EXPECT_EQ(sub2->count(), 1u);
+    EXPECT_EQ(sub3->count(), 1u);
+
+    // subs should remain the same since the duplicate would not be registered there
+    /*subs = app.get_subcommands();
+    EXPECT_EQ(subs.size(), 3u);
+    EXPECT_EQ(subs[0], sub1);
+    EXPECT_EQ(subs[1], sub2);
+    EXPECT_EQ(subs[2], sub3);*/
+}
@@ -163,6 +163,21 @@ TEST_F(TApp, FooFooProblem) {
     EXPECT_EQ(other_str, "");
 }
  
+TEST_F(TApp, DuplicateSubcommands) {
+
+    auto foo = app.add_subcommand("foo");
+
+    args = {"foo", "foo"};
+    run();
+    EXPECT_TRUE(*foo);
+    EXPECT_EQ(foo->count(), 2);
+
+    args = {"foo", "foo", "foo"};
+    run();
+    EXPECT_TRUE(*foo);
+    EXPECT_EQ(foo->count(), 3);
+}
+
 TEST_F(TApp, Callbacks) {
     auto sub1 = app.add_subcommand("sub1");
     sub1->callback([]() { throw CLI::Success(); });