Throw errors for duplicate subcommand names, with ignore case too

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Version 0.5 (in progress)
 
-* `->ignore_case()` added to subcommands, options, and `add_set_ignore_case`. Subcommand inherit setting from parent App on creation.
+* Throw `OptionAlreadyAdded` errors for matching subcommands or options, with ignore-case included, tests
+* `->ignore_case()` added to subcommands, options, and `add_set_ignore_case`. Subcommands inherit setting from parent App on creation.
 * Subcommands now can be "chained", that is, left over arguments can now include subcommands that then get parsed. Subcommands are now a list (`get_subcommands`). Added `got_subcommand(App_or_name)` to check for subcommands.
 * Added `.allow_extras()` to disable error on failure. Parse returns a vector of leftover options. Renamed error to `ExtrasError`, and now triggers on extra options too.
 * Added `require_subcommand` to `App`, to simplify forcing subcommands. Do **not** do `add_subcommand()->require_subcommand`, since that is the subcommand, not the master `App`.

README.md

@@ -41,7 +41,6 @@ This library was built to supply the Application object for the GooFit CUDA/OMP
 * Collect user feedback
     * Ini configuration support is basic (long options only, no vector support), is more needed?
 * Evaluate compatibility with [ROOT](https://root.cern.ch)'s TApplication object.
-* Throw error if `ignore_case` causes non-unique matches
 
 See the [changelog](./CHANGELOG.md) or [GitHub releases](https://github.com/henryiii/CLI11/releases) for details.
 
@@ -158,7 +157,7 @@ everything after that is positional only.
 
 ## Subcommands
 
-Subcommands are naturally supported, with an infinite depth. To add a subcommand, call the `add_subcommand` method with a name and an optional description. This gives a pointer to an `App` that behaves just like the main app, and can take options or further subcommands. Add `->ignore_case()` to a subcommand to allow any variation of caps to also be accepted. Children inherit the current setting from the parent.
+Subcommands are naturally supported, with an infinite depth. To add a subcommand, call the `add_subcommand` method with a name and an optional description. This gives a pointer to an `App` that behaves just like the main app, and can take options or further subcommands. Add `->ignore_case()` to a subcommand to allow any variation of caps to also be accepted. Children inherit the current setting from the parent. You cannot add multiple matching subcommand names at the same level (including ignore case).
 
 All `App`s have a `get_subcommands()` method, which returns a list of pointers to the subcommand passed on the command line. A simple compare of these pointers to each subcommand allows choosing based on subcommand, facilitated by a `got_subcommand(App_or_name) method that will check the list for you. 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 `.add_callback`. If you throw `CLI::Success`, 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.

include/CLI/App.hpp

@@ -31,7 +31,6 @@ enum class Classifer {NONE, POSITIONAL_MARK, SHORT, LONG, SUBCOMMAND};
 
 class App;
 
-typedef std::unique_ptr<Option> Option_p;
 typedef std::unique_ptr<App> App_p;
 
 /// Creates a command line program, with very few defaults.
@@ -39,6 +38,7 @@ typedef std::unique_ptr<App> App_p;
 *  add_option methods make it easy to prepare options. Remember to call `.start` before starting your
 * program, so that the options can be evaluated and the help option doesn't accidentally run your program. */
 class App {
+    friend Option;
 protected:
     
     std::string name;
@@ -63,6 +63,7 @@ protected:
     Option* ini_setting {nullptr};
 
     bool case_insensitive {false};
+    App* parent {nullptr};
    
 
 public:
@@ -124,7 +125,12 @@ public:
         subcommands.emplace_back(new App(description, help));
         subcommands.back()->name = name_;
         subcommands.back()->allow_extras();
+        subcommands.back()->parent = this;
         subcommands.back()->case_insensitive = case_insensitive;
+        for(const auto& subc : subcommands)
+            if(subc.get() != subcommands.back().get())
+                if(subc->check_name(subcommands.back()->name) || subcommands.back()->check_name(subc->name)) 
+                    throw OptionAlreadyAdded(subc->name);
         return subcommands.back().get();
     }
 
@@ -147,12 +153,13 @@ public:
             std::string description="", 
             bool defaulted=false
             ) {
-        Option myopt{name_, description, callback, defaulted};
+        Option myopt{name_, description, callback, defaulted, this};
+
         if(std::find_if(std::begin(options), std::end(options),
                     [&myopt](const Option_p &v){return *v == myopt;}) == std::end(options)) {
             options.emplace_back();
             Option_p& option = options.back();
-            option.reset(new Option(name_, description, callback, defaulted));
+            option.reset(new Option(name_, description, callback, defaulted, this));
             return option.get();
         } else
             throw OptionAlreadyAdded(myopt.get_name());
@@ -545,6 +552,12 @@ public:
     /// Ignore case
     App* ignore_case(bool value = true) {
         case_insensitive = value;
+        if(parent != nullptr) {
+            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;
     }
 

include/CLI/Option.hpp

@@ -8,6 +8,7 @@
 #include <vector>
 #include <tuple>
 #include <algorithm>
+#include <memory>
 #include <set>
 
 #include "CLI/Error.hpp"
@@ -19,8 +20,12 @@ namespace CLI {
 typedef std::vector<std::vector<std::string>> results_t;
 typedef std::function<bool(results_t)> callback_t;
 
+class Option;
 class App;
 
+typedef std::unique_ptr<Option> Option_p;
+
+
 class Option {
     friend App;
 protected:
@@ -48,16 +53,19 @@ protected:
     std::set<Option*> _excludes;
     std::string _envname;
 
-    // Results
+    /// Remember the parent app
+    App* parent;
+
+    /// Results of parsing
     results_t results;
 
-
-public:
-    Option(std::string name, std::string description = "", std::function<bool(results_t)> callback=[](results_t){return true;}, bool _default=true) :
-      description(description), callback(callback), _default(_default) {
+    /// Making an option by hand is not defined, it must be made by the App class
+    Option(std::string name, std::string description = "", std::function<bool(results_t)> callback=[](results_t){return true;}, bool _default=true, App* parent = nullptr) :
+      description(description), callback(callback), _default(_default), parent(parent) {
         std::tie(snames, lnames, pname) = detail::get_names(detail::split_names(name));
     }
 
+public:
 
     // This class is "true" if optio passed.
     operator bool() const {
@@ -211,12 +219,17 @@ public:
     /// If options share any of the same names, they are equal (not counting positional)
     bool operator== (const Option& other) const {
         for(const std::string &sname : snames)
-            for(const std::string &othersname : other.snames)
-                if(sname == othersname)
+            if(other.check_sname(sname))
                 return true;
         for(const std::string &lname : lnames)
-            for(const std::string &otherlname : other.lnames)
-                if(lname == otherlname)
+            if(other.check_lname(lname))
+                return true;
+        // We need to do the inverse, just in case we are ignore_case
+        for(const std::string &sname : other.snames)
+            if(check_sname(sname))
+                return true;
+        for(const std::string &lname : other.lnames)
+            if(check_lname(lname))
                 return true;
         return false;
     }
@@ -234,8 +247,14 @@ public:
     }
 
     /// Ignore case
+    /// 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) {
         case_insensitive = value;
+        for(const Option_p& opt : dynamic_cast<T*>(parent)->options)
+            if(opt.get() != this && *opt == *this)
+                throw OptionAlreadyAdded(opt->get_name());
         return this;
     }
 

tests/CMakeLists.txt

@@ -5,6 +5,7 @@ set(CLI_TESTS
     IniTest
     SimpleTest
     AppTest
+    CreationTest
     SubcommandTest
     HelpTest)
 

tests/CreationTest.cpp

@@ -0,0 +1,76 @@
+#include "app_helper.hpp"
+#include <stdlib.h>
+
+TEST_F(TApp, AddingExisting) {
+    app.add_flag("-c,--count");
+    EXPECT_THROW(app.add_flag("--cat,-c"), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, AddingExistingNoCase) {
+    app.add_flag("-C,--count")->ignore_case();
+    EXPECT_THROW(app.add_flag("--cat,-c"), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, AddingExistingNoCaseReversed) {
+    app.add_flag("-c,--count")->ignore_case();
+    EXPECT_THROW(app.add_flag("--cat,-C"), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, AddingExistingWithCase) {
+    app.add_flag("-c,--count");
+    EXPECT_NO_THROW(app.add_flag("--Cat,-C"));
+}
+
+TEST_F(TApp, AddingExistingWithCaseAfter) {
+    auto count = app.add_flag("-c,--count");
+    app.add_flag("--Cat,-C");
+
+    EXPECT_THROW(count->ignore_case(), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, AddingExistingWithCaseAfter2) {
+    app.add_flag("-c,--count");
+    auto cat = app.add_flag("--Cat,-C");
+
+    EXPECT_THROW(cat->ignore_case(), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, MultipleSubcomMatching) {
+    app.add_subcommand("first");
+    app.add_subcommand("second");
+    app.add_subcommand("Second");
+    EXPECT_THROW(app.add_subcommand("first"), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, MultipleSubcomMatchingWithCase) {
+    app.add_subcommand("first")->ignore_case();
+    EXPECT_THROW(app.add_subcommand("fIrst"), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, MultipleSubcomMatchingWithCaseFirst) {
+    app.ignore_case();
+    app.add_subcommand("first");
+    EXPECT_THROW(app.add_subcommand("fIrst"), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, MultipleSubcomMatchingWithCaseInplace) {
+    app.add_subcommand("first");
+    auto first = app.add_subcommand("fIrst");
+    
+    EXPECT_THROW(first->ignore_case(), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, MultipleSubcomMatchingWithCaseInplace2) {
+    auto first = app.add_subcommand("first");
+    app.add_subcommand("fIrst");
+    
+    EXPECT_THROW(first->ignore_case(), CLI::OptionAlreadyAdded);
+}
+
+TEST_F(TApp, MultipleSubcomNoMatchingInplace2) {
+    auto first = app.add_subcommand("first");
+    auto second = app.add_subcommand("second");
+    
+    EXPECT_NO_THROW(first->ignore_case());
+    EXPECT_NO_THROW(second->ignore_case());
+}