Add needs to subcommand (#317)

* add a needs method to the app/subcommand

* add some needs subcommand tests

* add a few more subcommand tests for needs and alias

* fix shadow warnings

* add some tests of the error pathways and fix a few anomalous conditions on the Option excludes function

* add needs and alias functions in the readme

* add some tests of ignore_case and underscore with the alias operations

* add a few more test cases for needs option groups

* add callback tests with needs and add a few comments in the readme

* update formatting

* add error checks on the aliases and restrictions on valid names for subcommands and aliases

* add checks for matching subcommands and improve error return values to include the offending name

* add some tests of the alias errors

* add some more tests to check subcommand name matching during addition

* add some additional tests and remove a redundant chunk of codes

* add some more checks of subcommand name overlap in option_groups

* allow disabled subcommand to bypass name matching check

README.md

@@ -414,21 +414,21 @@ After specifying a set of options, you can also specify "filter" functions of th
 Here are some examples
 of `IsMember`:
 
-     -   `CLI::IsMember({"choice1", "choice2"})`: Select from exact match to choices.
+-   `CLI::IsMember({"choice1", "choice2"})`: Select from exact match to choices.
-     -   `CLI::IsMember({"choice1", "choice2"}, CLI::ignore_case, CLI::ignore_underscore)`: Match things like `Choice_1`, too.
+-   `CLI::IsMember({"choice1", "choice2"}, CLI::ignore_case, CLI::ignore_underscore)`: Match things like `Choice_1`, too.
-     -  `CLI::IsMember(std::set<int>({2,3,4}))`: Most containers and types work; you just need `std::begin`, `std::end`, and `::value_type`.
+-   `CLI::IsMember(std::set<int>({2,3,4}))`: Most containers and types work; you just need `std::begin`, `std::end`, and `::value_type`.
-     -   `CLI::IsMember(std::map<std::string, TYPE>({{"one", 1}, {"two", 2}}))`: You can use maps; in `->transform()` these replace the matched value with the matched key.  The value member of the map is not used in `IsMember`, so it can be any type.
+-   `CLI::IsMember(std::map<std::string, TYPE>({{"one", 1}, {"two", 2}}))`: You can use maps; in `->transform()` these replace the matched value with the matched key.  The value member of the map is not used in `IsMember`, so it can be any type.
-     -   `auto p = std::make_shared<std::vector<std::string>>(std::initializer_list<std::string>("one", "two")); CLI::IsMember(p)`: You can modify `p` later.
+-   `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, in which case it is transformed, 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 (`Transformer` and `CheckedTransformer` are interchangeable in the examples)
 of `Transformer`:
 
-     -   `CLI::Transformer({{"key1", "map1"},{"key2","map2"}})`: Select from key values and produce map values.
+-   `CLI::Transformer({{"key1", "map1"},{"key2","map2"}})`: Select from key values and produce map values.
 
-     -  `CLI::Transformer(std::map<std::string,int>({"two",2},{"three",3},{"four",4}}))`: most maplike containers work,  the `::value_type` needs to produce a pair of some kind.
+-   `CLI::Transformer(std::map<std::string,int>({"two",2},{"three",3},{"four",4}}))`: most maplike containers work,  the `::value_type` needs to produce a pair of some kind.
-     -   `CLI::CheckedTransformer(std::map<std::string, int>({{"one", 1}, {"two", 2}}))`: You can use maps; in `->transform()` these replace the matched key with the value.  `CheckedTransformer` also requires that the value either match one of the keys or match one of known outputs.
+-   `CLI::CheckedTransformer(std::map<std::string, int>({{"one", 1}, {"two", 2}}))`: You can use maps; in `->transform()` these replace the matched key with the value.  `CheckedTransformer` also requires that the value either match one of the keys or match one of known outputs.
-     -   `auto p = std::make_shared<CLI::TransformPairs<std::string>>(std::initializer_list<std::pair<std::string,std::string>>({"key1", "map1"},{"key2","map2"})); CLI::Transformer(p)`: You can modify `p` later. `TransformPairs<T>` is an alias for `std::vector<std::pair<<std::string,T>>`
+-   `auto p = std::make_shared<CLI::TransformPairs<std::string>>(std::initializer_list<std::pair<std::string,std::string>>({"key1", "map1"},{"key2","map2"})); CLI::Transformer(p)`: You can modify `p` later. `TransformPairs<T>` is an alias for `std::vector<std::pair<<std::string,T>>`
 
 NOTES:  If the container used in `IsMember`, `Transformer`, or `CheckedTransformer` has a `find` function like `std::unordered_map`  or `std::map` then that function is used to do the searching. If it does not have a `find` function a linear search is performed.  If there are filters present, the fast search is performed first, and if that fails a linear search with the filters on the key values is performed.
 
@@ -535,6 +535,7 @@ There are several options that are supported on the main app and subcommands and
 -   `.enabled_by_default()`: 🆕 Specify that at the start of each parse the subcommand/option_group should be enabled.  This is useful for allowing some Subcommands to disable others.
 -   `.validate_positionals()`: 🆕 Specify that positionals should pass validation before matching.  Validation is specified through `transform`, `check`, and `each` for an option.  If an argument fails validation it is not an error and matching proceeds to the next available positional or extra arguments.
 -   `.excludes(option_or_subcommand)`: 🆕 If given an option pointer or pointer to another subcommand, these subcommands cannot be given together.  In the case of options, if the option is passed the subcommand cannot be used and will generate an error.
+-   `.needs(option_or_subcommand)`: 🚧 If given an option pointer or pointer to another subcommand, the subcommands will require the given option to have been given before this subcommand is validated which occurs prior to execution of any callback or after parsing is completed.
 -   `.require_option()`: 🆕 Require 1 or more options or option groups be used.
 -   `.require_option(N)`: 🆕 Require `N` options or option groups, if `N>0`, or up to `N` if `N<0`. `N=0` resets to the default to 0 or more.
 -   `.require_option(min, max)`: 🆕 Explicitly set min and max allowed options or option groups. Setting `max` to 0 implies unlimited options.
@@ -555,6 +556,7 @@ There are several options that are supported on the main app and subcommands and
 -   `.formatter(fmt)`: Set a formatter, with signature `std::string(const App*, std::string, AppFormatMode)`. See Formatting for more details.
 -   `.description(str)`: Set/change the description.
 -   `.get_description()`: Access the description.
+-   `.alias(str)`:🚧 set an alias for the subcommand, this allows subcommands to be called by more than one name.
 -   `.parsed()`: True if this subcommand was given on the command line.
 -   `.count()`: Returns the number of times the subcommand was called.
 -   `.count(option_name)`: Returns the number of times a particular option was called.

include/CLI/App.hpp

@@ -159,6 +159,14 @@ class App {
     /// not be
     std::set<Option *> exclude_options_;
 
+    /// this is a list of subcommands or option groups that are required by this one, the list is not mutual,  the
+    /// listed subcommands do not require this one
+    std::set<App *> need_subcommands_;
+
+    /// This is a list of options which are required by this app, the list is not mutual, listed options do not need the
+    /// subcommand not be
+    std::set<Option *> need_options_;
+
     ///@}
     /// @name Subcommands
     ///@{
@@ -213,6 +221,9 @@ class App {
     /// The group membership INHERITABLE
     std::string group_{"Subcommands"};
 
+    /// Alias names for the subcommand
+    std::vector<std::string> aliases_;
+
     ///@}
     /// @name Config
     ///@{
@@ -315,11 +326,42 @@ class App {
 
     /// Set a name for the app (empty will use parser to set the name)
     App *name(std::string app_name = "") {
+
+        if(parent_ != nullptr) {
+            auto oname = name_;
             name_ = app_name;
+            auto &res = _compare_subcommand_names(*this, *_get_fallthrough_parent());
+            if(!res.empty()) {
+                name_ = oname;
+                throw(OptionAlreadyAdded(app_name + " conflicts with existing subcommand names"));
+            }
+        } else {
+            name_ = app_name;
+        }
         has_automatic_name_ = false;
         return this;
     }
 
+    /// Set an alias for the app
+    App *alias(std::string app_name) {
+        if(!detail::valid_name_string(app_name)) {
+            throw(IncorrectConstruction("alias is not a valid name string"));
+        }
+
+        if(parent_ != nullptr) {
+            aliases_.push_back(app_name);
+            auto &res = _compare_subcommand_names(*this, *_get_fallthrough_parent());
+            if(!res.empty()) {
+                aliases_.pop_back();
+                throw(OptionAlreadyAdded("alias already matches an existing subcommand: " + app_name));
+            }
+        } else {
+            aliases_.push_back(app_name);
+        }
+
+        return this;
+    }
+
     /// Remove the error when extras are left over on the command line.
     App *allow_extras(bool allow = true) {
         allow_extras_ = allow;
@@ -386,13 +428,16 @@ class App {
 
     /// Ignore case. Subcommands inherit value.
     App *ignore_case(bool value = true) {
+        if(value && !ignore_case_) {
+            ignore_case_ = true;
+            auto *p = (parent_ != nullptr) ? _get_fallthrough_parent() : this;
+            auto &match = _compare_subcommand_names(*this, *p);
+            if(!match.empty()) {
+                ignore_case_ = false; // we are throwing so need to be exception invariant
+                throw OptionAlreadyAdded("ignore case would cause subcommand name conflicts: " + match);
+            }
+        }
         ignore_case_ = value;
-        if(parent_ != nullptr && !name_.empty()) {
-            for(const auto &subc : parent_->subcommands_) {
-                if(subc.get() != this && (this->check_name(subc->name_) || subc->check_name(this->name_)))
-                    throw OptionAlreadyAdded(subc->name_);
-            }
-        }
         return this;
     }
 
@@ -411,13 +456,16 @@ class App {
 
     /// Ignore underscore. Subcommands inherit value.
     App *ignore_underscore(bool value = true) {
+        if(value && !ignore_underscore_) {
+            ignore_underscore_ = true;
+            auto *p = (parent_ != nullptr) ? _get_fallthrough_parent() : this;
+            auto &match = _compare_subcommand_names(*this, *p);
+            if(!match.empty()) {
+                ignore_underscore_ = false;
+                throw OptionAlreadyAdded("ignore underscore would cause subcommand name conflicts: " + match);
+            }
+        }
         ignore_underscore_ = value;
-        if(parent_ != nullptr && !name_.empty()) {
-            for(const auto &subc : parent_->subcommands_) {
-                if(subc.get() != this && (this->check_name(subc->name_) || subc->check_name(this->name_)))
-                    throw OptionAlreadyAdded(subc->name_);
-            }
-        }
         return this;
     }
 
@@ -493,7 +541,17 @@ class App {
 
             return option.get();
         }
-        throw OptionAlreadyAdded(myopt.get_name());
+        // we know something matches now find what it is so we can produce more error information
+        for(auto &opt : options_) {
+            auto &matchname = opt->matching_name(myopt);
+            if(!matchname.empty()) {
+                throw(OptionAlreadyAdded("added option matched existing option name: " + matchname));
+            }
+        }
+        // this line should not be reached the above loop should trigger the throw
+        // LCOV_EXCL_START
+        throw(OptionAlreadyAdded("added option matched existing option name"));
+        // LCOV_EXCL_END
     }
 
     /// Add option for non-vectors (duplicate copy needed without defaulted to avoid `iostream << value`)
@@ -1033,6 +1091,9 @@ class App {
 
     /// Add a subcommand. Inherits INHERITABLE and OptionDefaults, and help flag
     App *add_subcommand(std::string subcommand_name = "", std::string subcommand_description = "") {
+        if(!subcommand_name.empty() && !detail::valid_name_string(subcommand_name)) {
+            throw IncorrectConstruction("subcommand name is not valid");
+        }
         CLI::App_p subcom = std::shared_ptr<App>(new App(std::move(subcommand_description), subcommand_name, this));
         return add_subcommand(std::move(subcom));
     }
@@ -1041,10 +1102,10 @@ class App {
     App *add_subcommand(CLI::App_p subcom) {
         if(!subcom)
             throw IncorrectConstruction("passed App is not valid");
-        if(!subcom->name_.empty()) {
+        auto ckapp = (name_.empty() && parent_ != nullptr) ? _get_fallthrough_parent() : this;
-            for(const auto &subc : subcommands_)
+        auto &mstrg = _compare_subcommand_names(*subcom, *ckapp);
-                if(subc->check_name(subcom->name_) || subcom->check_name(subc->name_))
+        if(!mstrg.empty()) {
-                    throw OptionAlreadyAdded(subc->name_);
+            throw(OptionAlreadyAdded("subcommand name or alias matches existing subcommand: " + mstrg));
         }
         subcom->parent_ = this;
         subcommands_.push_back(std::move(subcom));
@@ -1056,6 +1117,7 @@ class App {
         // Make sure no links exist
         for(App_p &sub : subcommands_) {
             sub->remove_excludes(subcom);
+            sub->remove_needs(subcom);
         }
 
         auto iterator = std::find_if(
@@ -1440,9 +1502,12 @@ class App {
 
     /// Sets excluded subcommands for the subcommand
     App *excludes(App *app) {
-        if((app == this) || (app == nullptr)) {
+        if(app == nullptr) {
             throw OptionNotFound("nullptr passed");
         }
+        if(app == this) {
+            throw OptionNotFound("cannot self reference in needs");
+        }
         auto res = exclude_subcommands_.insert(app);
         // subcommand exclusion should be symmetric
         if(res.second) {
@@ -1451,6 +1516,25 @@ class App {
         return this;
     }
 
+    App *needs(Option *opt) {
+        if(opt == nullptr) {
+            throw OptionNotFound("nullptr passed");
+        }
+        need_options_.insert(opt);
+        return this;
+    }
+
+    App *needs(App *app) {
+        if(app == nullptr) {
+            throw OptionNotFound("nullptr passed");
+        }
+        if(app == this) {
+            throw OptionNotFound("cannot self reference in needs");
+        }
+        need_subcommands_.insert(app);
+        return this;
+    }
+
     /// Removes an option from the excludes list of this subcommand
     bool remove_excludes(Option *opt) {
         auto iterator = std::find(std::begin(exclude_options_), std::end(exclude_options_), opt);
@@ -1461,7 +1545,7 @@ class App {
         return true;
     }
 
-    /// Removes a subcommand from this excludes list of this subcommand
+    /// Removes a subcommand from the excludes list of this subcommand
     bool remove_excludes(App *app) {
         auto iterator = std::find(std::begin(exclude_subcommands_), std::end(exclude_subcommands_), app);
         if(iterator == std::end(exclude_subcommands_)) {
@@ -1473,6 +1557,26 @@ class App {
         return true;
     }
 
+    /// Removes an option from the needs list of this subcommand
+    bool remove_needs(Option *opt) {
+        auto iterator = std::find(std::begin(need_options_), std::end(need_options_), opt);
+        if(iterator == std::end(need_options_)) {
+            return false;
+        }
+        need_options_.erase(iterator);
+        return true;
+    }
+
+    /// Removes a subcommand from the needs list of this subcommand
+    bool remove_needs(App *app) {
+        auto iterator = std::find(std::begin(need_subcommands_), std::end(need_subcommands_), app);
+        if(iterator == std::end(need_subcommands_)) {
+            return false;
+        }
+        need_subcommands_.erase(iterator);
+        return true;
+    }
+
     ///@}
     /// @name Help
     ///@{
@@ -1688,7 +1792,16 @@ class App {
     const App *get_parent() const { return parent_; }
 
     /// Get the name of the current app
-    std::string get_name() const { return name_; }
+    const std::string &get_name() const { return name_; }
+
+    /// Get the aliases of the current app
+    const std::vector<std::string> &get_aliases() const { return aliases_; }
+
+    /// clear all the aliases of the current App
+    App *clear_aliases() {
+        aliases_.clear();
+        return this;
+    }
 
     /// Get a display name for an app
     std::string get_display_name() const { return (!name_.empty()) ? name_ : "[Option Group: " + get_group() + "]"; }
@@ -1705,7 +1818,21 @@ class App {
             name_to_check = detail::to_lower(name_to_check);
         }
 
-        return local_name == name_to_check;
+        if(local_name == name_to_check) {
+            return true;
+        }
+        for(auto les : aliases_) {
+            if(ignore_underscore_) {
+                les = detail::remove_underscore(les);
+            }
+            if(ignore_case_) {
+                les = detail::to_lower(les);
+            }
+            if(les == name_to_check) {
+                return true;
+            }
+        }
+        return false;
     }
 
     /// Get the groups available directly from this option (in order)
@@ -2026,6 +2153,30 @@ class App {
             // if we are excluded but didn't receive anything, just return
             return;
         }
+
+        // check excludes
+        bool missing_needed{false};
+        std::string missing_need;
+        for(auto &opt : need_options_) {
+            if(opt->count() == 0) {
+                missing_needed = true;
+                missing_need = opt->get_name();
+            }
+        }
+        for(auto &subc : need_subcommands_) {
+            if(subc->count_all() == 0) {
+                missing_needed = true;
+                missing_need = subc->get_display_name();
+            }
+        }
+        if(missing_needed) {
+            if(count_all() > 0) {
+                throw RequiresError(get_display_name(), missing_need);
+            }
+            // if we missing something but didn't have any options, just return
+            return;
+        }
+
         size_t used_options = 0;
         for(const Option_p &opt : options_) {
 
@@ -2287,7 +2438,7 @@ class App {
             break;
         case detail::Classifier::NONE:
             // Probably a positional or something for a parent (sub)command
-            retval = _parse_positional(args);
+            retval = _parse_positional(args, false);
             if(retval && positionals_at_end_) {
                 positional_only = true;
             }
@@ -2327,8 +2478,9 @@ class App {
     }
 
     /// Parse a positional, go up the tree to check
+    /// @param haltOnSubcommand if set to true the operation will not process subcommands merely return false
     /// Return true if the positional was used false otherwise
-    bool _parse_positional(std::vector<std::string> &args) {
+    bool _parse_positional(std::vector<std::string> &args, bool haltOnSubcommand) {
 
         const std::string &positional = args.back();
 
@@ -2377,7 +2529,7 @@ class App {
 
         for(auto &subc : subcommands_) {
             if((subc->name_.empty()) && (!subc->disabled_)) {
-                if(subc->_parse_positional(args)) {
+                if(subc->_parse_positional(args, false)) {
                     if(!subc->pre_parse_called_) {
                         subc->_trigger_pre_parse(args.size());
                     }
@@ -2387,11 +2539,14 @@ class App {
         }
         // let the parent deal with it if possible
         if(parent_ != nullptr && fallthrough_)
-            return _get_fallthrough_parent()->_parse_positional(args);
+            return _get_fallthrough_parent()->_parse_positional(args, static_cast<bool>(parse_complete_callback_));
 
         /// Try to find a local subcommand that is repeated
         auto com = _find_subcommand(args.back(), true, false);
         if(com != nullptr && (require_subcommand_max_ == 0 || require_subcommand_max_ > parsed_subcommands_.size())) {
+            if(haltOnSubcommand) {
+                return false;
+            }
             args.pop_back();
             com->_parse(args);
             return true;
@@ -2436,7 +2591,8 @@ class App {
                 if(subc != nullptr) {
                     return subc;
                 }
-            } else if(com->check_name(subc_name)) {
+            }
+            if(com->check_name(subc_name)) {
                 if((!*com) || !ignore_used)
                     return com.get();
             }
@@ -2450,7 +2606,7 @@ class App {
     /// return true if the subcommand was processed false otherwise
     bool _parse_subcommand(std::vector<std::string> &args) {
         if(_count_remaining_positionals(/* required */ true) > 0) {
-            _parse_positional(args);
+            _parse_positional(args, false);
             return true;
         }
         auto com = _find_subcommand(args.back(), true, true);
@@ -2645,6 +2801,56 @@ class App {
         return fallthrough_parent;
     }
 
+    /// Helper function to run through all possible comparisons of subcommand names to check there is no overlap
+    const std::string &_compare_subcommand_names(const App &subcom, const App &base) const {
+        static const std::string estring;
+        if(subcom.disabled_) {
+            return estring;
+        }
+        for(auto &subc : base.subcommands_) {
+            if(subc.get() != &subcom) {
+                if(subc->disabled_) {
+                    continue;
+                }
+                if(!subcom.get_name().empty()) {
+                    if(subc->check_name(subcom.get_name())) {
+                        return subcom.get_name();
+                    }
+                }
+                if(!subc->get_name().empty()) {
+                    if(subcom.check_name(subc->get_name())) {
+                        return subc->get_name();
+                    }
+                }
+                for(const auto &les : subcom.aliases_) {
+                    if(subc->check_name(les)) {
+                        return les;
+                    }
+                }
+                // this loop is needed in case of ignore_underscore or ignore_case on one but not the other
+                for(const auto &les : subc->aliases_) {
+                    if(subcom.check_name(les)) {
+                        return les;
+                    }
+                }
+                // if the subcommand is an option group we need to check deeper
+                if(subc->get_name().empty()) {
+                    auto &cmpres = _compare_subcommand_names(subcom, *subc);
+                    if(!cmpres.empty()) {
+                        return cmpres;
+                    }
+                }
+                // if the test subcommand is an option group we need to check deeper
+                if(subcom.get_name().empty()) {
+                    auto &cmpres = _compare_subcommand_names(*subc, subcom);
+                    if(!cmpres.empty()) {
+                        return cmpres;
+                    }
+                }
+            }
+        }
+        return estring;
+    }
     /// Helper function to place extra values in the most appropriate position
     void _move_to_missing(detail::Classifier val_type, const std::string &val) {
         if(allow_extras_ || subcommands_.empty()) {
@@ -2696,10 +2902,10 @@ class App {
                 app->options_.push_back(std::move(*iterator));
                 options_.erase(iterator);
             } else {
-                throw OptionAlreadyAdded(opt->get_name());
+                throw OptionAlreadyAdded("option was not located: " + opt->get_name());
             }
         } else {
-            throw OptionNotFound("could not locate the given App");
+            throw OptionNotFound("could not locate the given Option");
         }
     }
 }; // namespace CLI

include/CLI/Option.hpp

@@ -421,19 +421,20 @@ class Option : public OptionBase<Option> {
 
     /// Sets required options
     Option *needs(Option *opt) {
-        auto tup = needs_.insert(opt);
+        if(opt != this) {
-        if(!tup.second)
+            needs_.insert(opt);
-            throw OptionAlreadyAdded::Requires(get_name(), opt->get_name());
+        }
         return this;
     }
 
     /// Can find a string if needed
     template <typename T = App> Option *needs(std::string opt_name) {
-        for(const Option_p &opt : dynamic_cast<T *>(parent_)->options_)
+        auto opt = dynamic_cast<T *>(parent_)->get_option_no_throw(opt_name);
-            if(opt.get() != this && opt->check_name(opt_name))
+        if(opt == nullptr) {
-                return needs(opt.get());
             throw IncorrectConstruction::MissingOption(opt_name);
         }
+        return needs(opt);
+    }
 
     /// Any number supported, any mix of string and Opt
     template <typename A, typename B, typename... ARG> Option *needs(A opt, B opt1, ARG... args) {
@@ -454,6 +455,9 @@ class Option : public OptionBase<Option> {
 
     /// Sets excluded options
     Option *excludes(Option *opt) {
+        if(opt == this) {
+            throw(IncorrectConstruction("and option cannot exclude itself"));
+        }
         excludes_.insert(opt);
 
         // Help text should be symmetric - excluding a should exclude b
@@ -467,11 +471,12 @@ class Option : public OptionBase<Option> {
 
     /// Can find a string if needed
     template <typename T = App> Option *excludes(std::string opt_name) {
-        for(const Option_p &opt : dynamic_cast<T *>(parent_)->options_)
+        auto opt = dynamic_cast<T *>(parent_)->get_option_no_throw(opt_name);
-            if(opt.get() != this && opt->check_name(opt_name))
+        if(opt == nullptr) {
-                return excludes(opt.get());
             throw IncorrectConstruction::MissingOption(opt_name);
         }
+        return excludes(opt);
+    }
 
     /// Any number supported, any mix of string and Opt
     template <typename A, typename B, typename... ARG> Option *excludes(A opt, B opt1, ARG... args) {
@@ -501,13 +506,22 @@ class Option : public OptionBase<Option> {
     /// The template hides the fact that we don't have the definition of App yet.
     /// You are never expected to add an argument to the template here.
     template <typename T = App> Option *ignore_case(bool value = true) {
+        if(!ignore_case_ && value) {
             ignore_case_ = value;
             auto *parent = dynamic_cast<T *>(parent_);
-
+            for(const Option_p &opt : parent->options_) {
-        for(const Option_p &opt : parent->options_)
+                if(opt.get() == this) {
-            if(opt.get() != this && *opt == *this)
+                    continue;
-                throw OptionAlreadyAdded(opt->get_name(true, true));
+                }
-
+                auto &omatch = opt->matching_name(*this);
+                if(!omatch.empty()) {
+                    ignore_case_ = false;
+                    throw OptionAlreadyAdded("adding ignore case caused a name conflict with " + omatch);
+                }
+            }
+        } else {
+            ignore_case_ = value;
+        }
         return this;
     }
 
@@ -516,12 +530,23 @@ class Option : public OptionBase<Option> {
     /// The template hides the fact that we don't have the definition of App yet.
     /// You are never expected to add an argument to the template here.
     template <typename T = App> Option *ignore_underscore(bool value = true) {
+
+        if(!ignore_underscore_ && value) {
             ignore_underscore_ = value;
             auto *parent = dynamic_cast<T *>(parent_);
-        for(const Option_p &opt : parent->options_)
+            for(const Option_p &opt : parent->options_) {
-            if(opt.get() != this && *opt == *this)
+                if(opt.get() == this) {
-                throw OptionAlreadyAdded(opt->get_name(true, true));
+                    continue;
-
+                }
+                auto &omatch = opt->matching_name(*this);
+                if(!omatch.empty()) {
+                    ignore_underscore_ = false;
+                    throw OptionAlreadyAdded("adding ignore underscore caused a name conflict with " + omatch);
+                }
+            }
+        } else {
+            ignore_underscore_ = value;
+        }
         return this;
     }
 
@@ -746,26 +771,29 @@ class Option : public OptionBase<Option> {
             throw ConversionError(get_name(), results_);
     }
 
-    /// If options share any of the same names, they are equal (not counting positional)
+    /// If options share any of the same names, find it
-    bool operator==(const Option &other) const {
+    const std::string &matching_name(const Option &other) const {
+        static const std::string estring;
         for(const std::string &sname : snames_)
             if(other.check_sname(sname))
-                return true;
+                return sname;
         for(const std::string &lname : lnames_)
             if(other.check_lname(lname))
-                return true;
+                return lname;
 
         if(ignore_case_ ||
            ignore_underscore_) { // We need to do the inverse, in case we are ignore_case or ignore underscore
             for(const std::string &sname : other.snames_)
                 if(check_sname(sname))
-                    return true;
+                    return sname;
             for(const std::string &lname : other.lnames_)
                 if(check_lname(lname))
-                    return true;
+                    return lname;
         }
-        return false;
+        return estring;
     }
+    /// If options share any of the same names, they are equal (not counting positional)
+    bool operator==(const Option &other) const { return !matching_name(other).empty(); }
 
     /// Check a name. Requires "-" or "--" for short / long, supports positional name
     bool check_name(std::string name) const {

tests/AppTest.cpp

@@ -1792,6 +1792,8 @@ TEST_F(TApp, NeedsFlags) {
 
     args = {"--both"};
     EXPECT_THROW(run(), CLI::RequiresError);
+
+    EXPECT_NO_THROW(opt->needs(opt));
 }
 
 TEST_F(TApp, ExcludesFlags) {
@@ -1811,6 +1813,8 @@ TEST_F(TApp, ExcludesFlags) {
 
     args = {"--string", "--nostr"};
     EXPECT_THROW(run(), CLI::ExcludesError);
+
+    EXPECT_THROW(opt->excludes(opt), CLI::IncorrectConstruction);
 }
 
 TEST_F(TApp, ExcludesMixedFlags) {

tests/CreationTest.cpp

@@ -227,14 +227,16 @@ TEST_F(TApp, IncorrectConstructionDuplicateNeeds) {
     auto cat = app.add_flag("--cat");
     auto other = app.add_flag("--other");
     ASSERT_NO_THROW(cat->needs(other));
-    EXPECT_THROW(cat->needs(other), CLI::OptionAlreadyAdded);
+    // duplicated needs is redundant but not an error
+    EXPECT_NO_THROW(cat->needs(other));
 }
 
 TEST_F(TApp, IncorrectConstructionDuplicateNeedsTxt) {
     auto cat = app.add_flag("--cat");
     app.add_flag("--other");
     ASSERT_NO_THROW(cat->needs("--other"));
-    EXPECT_THROW(cat->needs("--other"), CLI::OptionAlreadyAdded);
+    // duplicate needs is redundant but not an error
+    EXPECT_NO_THROW(cat->needs("--other"));
 }
 
 // Now allowed

tests/OptionGroupTest.cpp

@@ -438,6 +438,52 @@ TEST_F(ManyGroups, ExcludesGroup) {
     EXPECT_FALSE(g1->remove_excludes(g2));
 }
 
+TEST_F(ManyGroups, NeedsGroup) {
+    remove_required();
+    // all groups needed if g1 is used
+    g1->needs(g2);
+    g1->needs(g3);
+    args = {"--name1", "test"};
+    EXPECT_THROW(run(), CLI::RequiresError);
+    // other groups should run fine
+    args = {"--name2", "test2"};
+
+    run();
+    // all three groups should be fine
+    args = {"--name1", "test", "--name2", "test2", "--name3", "test3"};
+
+    EXPECT_NO_THROW(run());
+}
+
+// test adding an option group with existing subcommands to an app
+TEST_F(TApp, ExistingSubcommandMatch) {
+    auto sshared = std::make_shared<CLI::Option_group>("documenting the subcommand", "sub1g", nullptr);
+    auto s1 = sshared->add_subcommand("sub1");
+    auto o1 = sshared->add_option_group("opt1");
+    o1->add_subcommand("sub3")->alias("sub4");
+
+    app.add_subcommand("sub1");
+
+    try {
+        app.add_subcommand(sshared);
+        // this should throw the next line should never be reached
+        EXPECT_FALSE(true);
+    } catch(const CLI::OptionAlreadyAdded &oaa) {
+        EXPECT_THAT(oaa.what(), HasSubstr("sub1"));
+    }
+    sshared->remove_subcommand(s1);
+
+    app.add_subcommand("sub3");
+    // now check that the subsubcommand overlaps
+    try {
+        app.add_subcommand(sshared);
+        // this should throw the next line should never be reached
+        EXPECT_FALSE(true);
+    } catch(const CLI::OptionAlreadyAdded &oaa) {
+        EXPECT_THAT(oaa.what(), HasSubstr("sub3"));
+    }
+}
+
 TEST_F(ManyGroups, SingleGroupError) {
     // only 1 group can be used
     main->require_option(1);
@@ -503,7 +549,7 @@ TEST_F(ManyGroups, RequiredFirst) {
 }
 
 TEST_F(ManyGroups, DisableFirst) {
-    // only 1 group can be used
+    // only 1 group can be used if remove_required not used
     remove_required();
     g1->disabled();
 
@@ -521,12 +567,15 @@ TEST_F(ManyGroups, DisableFirst) {
 }
 
 TEST_F(ManyGroups, SameSubcommand) {
-    // only 1 group can be used
+    // only 1 group can be used if remove_required not used
     remove_required();
-    auto sub1 = g1->add_subcommand("sub1");
+    auto sub1 = g1->add_subcommand("sub1")->disabled();
-    auto sub2 = g2->add_subcommand("sub1");
+    auto sub2 = g2->add_subcommand("sub1")->disabled();
     auto sub3 = g3->add_subcommand("sub1");
-
+    // so when the subcommands are disabled they can have the same name
+    sub1->disabled(false);
+    sub2->disabled(false);
+    // if they are reenabled they are not checked for overlap on enabling so they can have the same name
     args = {"sub1", "sub1", "sub1"};
 
     run();
@@ -534,7 +583,6 @@ TEST_F(ManyGroups, SameSubcommand) {
     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);
@@ -556,7 +604,7 @@ TEST_F(ManyGroups, SameSubcommand) {
     EXPECT_EQ(subs[2], sub3);
 }
 TEST_F(ManyGroups, CallbackOrder) {
-    // only 1 group can be used
+    // only 1 group can be used if remove_required not used
     remove_required();
     std::vector<int> callback_order;
     g1->callback([&callback_order]() { callback_order.push_back(1); });
@@ -582,6 +630,7 @@ TEST_F(ManyGroups, CallbackOrder) {
 
 // Test the fallthrough for extra arguments
 TEST_F(ManyGroups, ExtrasFallDown) {
+    // only 1 group can be used if remove_required not used
     remove_required();
 
     args = {"--test1", "--flag", "extra"};

tests/SubcommandTest.cpp

@@ -93,6 +93,17 @@ TEST_F(TApp, MultiSubFallthrough) {
     EXPECT_THROW(app.got_subcommand("sub3"), CLI::OptionNotFound);
 }
 
+TEST_F(TApp, CrazyNameSubcommand) {
+    auto sub1 = app.add_subcommand("sub1");
+    // name can be set to whatever
+    EXPECT_NO_THROW(sub1->name("crazy name with spaces"));
+    args = {"crazy name with spaces"};
+    run();
+
+    EXPECT_TRUE(app.got_subcommand("crazy name with spaces"));
+    EXPECT_EQ(sub1->count(), 1u);
+}
+
 TEST_F(TApp, RequiredAndSubcoms) { // #23
 
     std::string baz;
@@ -272,6 +283,46 @@ TEST_F(TApp, CallbackOrder) {
     EXPECT_EQ(cb[6], "c2");
     EXPECT_EQ(cb[7], "ac2");
 }
+
+TEST_F(TApp, CallbackOrder2) {
+
+    std::vector<std::string> cb;
+    app.add_subcommand("sub1")->parse_complete_callback([&cb]() { cb.push_back("sub1"); });
+    app.add_subcommand("sub2")->parse_complete_callback([&cb]() { cb.push_back("sub2"); });
+    app.add_subcommand("sub3")->parse_complete_callback([&cb]() { cb.push_back("sub3"); });
+
+    args = {"sub1", "sub2", "sub3", "sub1", "sub1", "sub2", "sub1"};
+    run();
+    EXPECT_EQ(cb.size(), 7u);
+    EXPECT_EQ(cb[0], "sub1");
+    EXPECT_EQ(cb[1], "sub2");
+    EXPECT_EQ(cb[2], "sub3");
+    EXPECT_EQ(cb[3], "sub1");
+    EXPECT_EQ(cb[4], "sub1");
+    EXPECT_EQ(cb[5], "sub2");
+    EXPECT_EQ(cb[6], "sub1");
+}
+
+TEST_F(TApp, CallbackOrder2_withFallthrough) {
+
+    std::vector<std::string> cb;
+
+    app.add_subcommand("sub1")->parse_complete_callback([&cb]() { cb.push_back("sub1"); })->fallthrough();
+    app.add_subcommand("sub2")->parse_complete_callback([&cb]() { cb.push_back("sub2"); });
+    app.add_subcommand("sub3")->parse_complete_callback([&cb]() { cb.push_back("sub3"); });
+
+    args = {"sub1", "sub2", "sub3", "sub1", "sub1", "sub2", "sub1"};
+    run();
+    EXPECT_EQ(cb.size(), 7u);
+    EXPECT_EQ(cb[0], "sub1");
+    EXPECT_EQ(cb[1], "sub2");
+    EXPECT_EQ(cb[2], "sub3");
+    EXPECT_EQ(cb[3], "sub1");
+    EXPECT_EQ(cb[4], "sub1");
+    EXPECT_EQ(cb[5], "sub2");
+    EXPECT_EQ(cb[6], "sub1");
+}
+
 TEST_F(TApp, RuntimeErrorInCallback) {
     auto sub1 = app.add_subcommand("sub1");
     sub1->callback([]() { throw CLI::RuntimeError(); });
@@ -420,7 +471,7 @@ TEST_F(TApp, Nameless4LayerDeep) {
 }
 
 /// Put subcommands in some crazy pattern and make everything still works
-TEST_F(TApp, Nameless4LayerDeepMulit) {
+TEST_F(TApp, Nameless4LayerDeepMulti) {
 
     auto sub1 = app.add_subcommand();
     auto sub2 = app.add_subcommand();
@@ -1248,6 +1299,93 @@ TEST_F(ManySubcommands, SubcommandOptionExclusion) {
     }
 }
 
+TEST_F(ManySubcommands, SubcommandNeeds) {
+
+    sub1->needs(sub2);
+    args = {"sub1", "sub2"};
+    EXPECT_NO_THROW(run());
+
+    args = {"sub2"};
+    EXPECT_NO_THROW(run());
+
+    args = {"sub1"};
+    EXPECT_THROW(run(), CLI::RequiresError);
+
+    sub1->needs(sub3);
+    args = {"sub1", "sub2", "sub3"};
+    EXPECT_NO_THROW(run());
+
+    args = {"sub1", "sub2", "sub4"};
+    EXPECT_THROW(run(), CLI::RequiresError);
+
+    args = {"sub1", "sub2", "sub4"};
+    sub1->remove_needs(sub3);
+    EXPECT_NO_THROW(run());
+}
+
+TEST_F(ManySubcommands, SubcommandNeedsOptions) {
+
+    auto opt = app.add_flag("--subactive");
+    sub1->needs(opt);
+    sub1->fallthrough();
+    args = {"sub1", "--subactive"};
+    EXPECT_NO_THROW(run());
+
+    args = {"sub1"};
+    EXPECT_THROW(run(), CLI::RequiresError);
+
+    args = {"--subactive"};
+    EXPECT_NO_THROW(run());
+
+    auto opt2 = app.add_flag("--subactive2");
+
+    sub1->needs(opt2);
+    args = {"sub1", "--subactive"};
+    EXPECT_THROW(run(), CLI::RequiresError);
+
+    args = {"--subactive", "--subactive2", "sub1"};
+    EXPECT_NO_THROW(run());
+
+    sub1->remove_needs(opt2);
+    args = {"sub1", "--subactive"};
+    EXPECT_NO_THROW(run());
+}
+
+TEST_F(ManySubcommands, SubcommandNeedsOptionsCallbackOrdering) {
+    int count = 0;
+    auto opt = app.add_flag("--subactive");
+    app.add_flag("--flag1");
+    sub1->needs(opt);
+    sub1->fallthrough();
+    sub1->parse_complete_callback([&count]() { ++count; });
+    args = {"sub1", "--flag1", "sub1", "--subactive"};
+    EXPECT_THROW(run(), CLI::RequiresError);
+    // the subcommand has to pass validation by the first callback
+    sub1->immediate_callback(false);
+    // now since the callback executes after
+
+    EXPECT_NO_THROW(run());
+    EXPECT_EQ(count, 1);
+    sub1->immediate_callback();
+    args = {"--subactive", "sub1"};
+    // now the required is processed first
+    EXPECT_NO_THROW(run());
+}
+
+TEST_F(ManySubcommands, SubcommandNeedsFail) {
+
+    auto opt = app.add_flag("--subactive");
+    auto opt2 = app.add_flag("--dummy");
+    sub1->needs(opt);
+    EXPECT_THROW(sub1->needs((CLI::Option *)nullptr), CLI::OptionNotFound);
+    EXPECT_THROW(sub1->needs((CLI::App *)nullptr), CLI::OptionNotFound);
+    EXPECT_THROW(sub1->needs(sub1), CLI::OptionNotFound);
+
+    EXPECT_TRUE(sub1->remove_needs(opt));
+    EXPECT_FALSE(sub1->remove_needs(opt2));
+    EXPECT_FALSE(sub1->remove_needs(sub1));
+}
+
 TEST_F(ManySubcommands, SubcommandRequired) {
 
     sub1->required();
@@ -1377,6 +1515,176 @@ TEST_F(TApp, UnnamedSubNoExtras) {
     EXPECT_EQ(sub->remaining_size(), 0u);
 }
 
+TEST_F(TApp, SubcommandAlias) {
+    double val;
+    auto sub = app.add_subcommand("sub1");
+    sub->alias("sub2");
+    sub->alias("sub3");
+    sub->add_option("-v,--value", val);
+    args = {"sub1", "-v", "-3"};
+    run();
+    EXPECT_EQ(val, -3.0);
+
+    args = {"sub2", "--value", "-5"};
+    run();
+    EXPECT_EQ(val, -5.0);
+
+    args = {"sub3", "-v", "7"};
+    run();
+    EXPECT_EQ(val, 7);
+
+    auto &al = sub->get_aliases();
+    ASSERT_GE(al.size(), 2U);
+
+    EXPECT_EQ(al[0], "sub2");
+    EXPECT_EQ(al[1], "sub3");
+
+    sub->clear_aliases();
+    EXPECT_TRUE(al.empty());
+}
+
+TEST_F(TApp, SubcommandAliasIgnoreCaseUnderscore) {
+    double val;
+    auto sub = app.add_subcommand("sub1");
+    sub->alias("sub2");
+    sub->alias("sub3");
+    sub->ignore_case();
+    sub->add_option("-v,--value", val);
+    args = {"sub1", "-v", "-3"};
+    run();
+    EXPECT_EQ(val, -3.0);
+
+    args = {"SUB2", "--value", "-5"};
+    run();
+    EXPECT_EQ(val, -5.0);
+
+    args = {"sUb3", "-v", "7"};
+    run();
+    EXPECT_EQ(val, 7);
+    sub->ignore_underscore();
+    args = {"sub_1", "-v", "-3"};
+    run();
+    EXPECT_EQ(val, -3.0);
+
+    args = {"SUB_2", "--value", "-5"};
+    run();
+    EXPECT_EQ(val, -5.0);
+
+    args = {"sUb_3", "-v", "7"};
+    run();
+    EXPECT_EQ(val, 7);
+
+    sub->ignore_case(false);
+    args = {"sub_1", "-v", "-3"};
+    run();
+    EXPECT_EQ(val, -3.0);
+
+    args = {"SUB_2", "--value", "-5"};
+    EXPECT_THROW(run(), CLI::ExtrasError);
+
+    args = {"sUb_3", "-v", "7"};
+    EXPECT_THROW(run(), CLI::ExtrasError);
+}
+
+TEST_F(TApp, OptionGroupAlias) {
+    double val;
+    auto sub = app.add_option_group("sub1");
+    sub->alias("sub2");
+    sub->alias("sub3");
+    sub->add_option("-v,--value", val);
+    args = {"sub1", "-v", "-3"};
+    EXPECT_THROW(run(), CLI::ExtrasError);
+
+    args = {"sub2", "--value", "-5"};
+    run();
+    EXPECT_EQ(val, -5.0);
+
+    args = {"sub3", "-v", "7"};
+    run();
+    EXPECT_EQ(val, 7);
+
+    args = {"-v", "-3"};
+    run();
+    EXPECT_EQ(val, -3);
+}
+
+TEST_F(TApp, AliasErrors) {
+    auto sub1 = app.add_subcommand("sub1");
+    auto sub2 = app.add_subcommand("sub2");
+
+    EXPECT_THROW(sub2->alias("this is a not a valid alias"), CLI::IncorrectConstruction);
+    EXPECT_THROW(sub2->alias("-alias"), CLI::IncorrectConstruction);
+    EXPECT_THROW(sub2->alias("alia$"), CLI::IncorrectConstruction);
+
+    EXPECT_THROW(app.add_subcommand("--bad_subcommand_name", "documenting the bad subcommand"),
+                 CLI::IncorrectConstruction);
+
+    EXPECT_THROW(app.add_subcommand("documenting a subcommand", "sub3"), CLI::IncorrectConstruction);
+    // cannot alias to an existing subcommand
+    EXPECT_THROW(sub2->alias("sub1"), CLI::OptionAlreadyAdded);
+    EXPECT_THROW(sub1->alias("sub2"), CLI::OptionAlreadyAdded);
+    // aliasing to an existing name should be allowed
+    EXPECT_NO_THROW(sub1->alias(sub1->get_name()));
+
+    sub1->alias("les1")->alias("les2")->alias("les_3");
+    sub2->alias("s2les1")->alias("s2les2")->alias("s2les3");
+
+    EXPECT_THROW(sub2->alias("les2"), CLI::OptionAlreadyAdded);
+    EXPECT_THROW(sub1->alias("s2les2"), CLI::OptionAlreadyAdded);
+
+    EXPECT_THROW(sub2->name("sub1"), CLI::OptionAlreadyAdded);
+    sub2->ignore_underscore();
+    EXPECT_THROW(sub2->alias("les3"), CLI::OptionAlreadyAdded);
+}
+// test adding a subcommand via the pointer
+TEST_F(TApp, ExistingSubcommandMatch) {
+    auto sshared = std::make_shared<CLI::App>("documenting the subcommand", "sub1");
+    sshared->alias("sub2")->alias("sub3");
+
+    EXPECT_EQ(sshared->get_name(), "sub1");
+    app.add_subcommand("sub1");
+
+    try {
+        app.add_subcommand(sshared);
+        // this should throw the next line should never be reached
+        EXPECT_FALSE(true);
+    } catch(const CLI::OptionAlreadyAdded &oaa) {
+        EXPECT_THAT(oaa.what(), HasSubstr("sub1"));
+    }
+    sshared->name("osub");
+    app.add_subcommand("sub2");
+    // now check that the aliases don't overlap
+    try {
+        app.add_subcommand(sshared);
+        // this should throw the next line should never be reached
+        EXPECT_FALSE(true);
+    } catch(const CLI::OptionAlreadyAdded &oaa) {
+        EXPECT_THAT(oaa.what(), HasSubstr("sub2"));
+    }
+    // now check that disabled subcommands can be added regardless of name
+    sshared->name("sub1");
+    sshared->disabled();
+    EXPECT_NO_THROW(app.add_subcommand(sshared));
+}
+
+TEST_F(TApp, AliasErrorsInOptionGroup) {
+    auto sub1 = app.add_subcommand("sub1");
+    auto g2 = app.add_option_group("g1");
+    auto sub2 = g2->add_subcommand("sub2");
+
+    // cannot alias to an existing subcommand even if it is in an option group
+    EXPECT_THROW(sub2->alias("sub1"), CLI::OptionAlreadyAdded);
+    EXPECT_THROW(sub1->alias("sub2"), CLI::OptionAlreadyAdded);
+
+    sub1->alias("les1")->alias("les2")->alias("les3");
+    sub2->alias("s2les1")->alias("s2les2")->alias("s2les3");
+
+    EXPECT_THROW(sub2->alias("les2"), CLI::OptionAlreadyAdded);
+    EXPECT_THROW(sub1->alias("s2les2"), CLI::OptionAlreadyAdded);
+
+    EXPECT_THROW(sub2->name("sub1"), CLI::OptionAlreadyAdded);
+}
+
 TEST(SharedSubTests, SharedSubcommand) {
     double val, val2, val3, val4;
     CLI::App app1{"test program1"};