feat: relaxed option naming (#627)

* add a test for std::map * add some test of the relaxed naming and other checks * add validator for aliases, group names and option groups * add extra tests and update readme * style: pre-commit.ci fixes * update the book chapters * fix codacy issue * Apply suggestions from code review Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>

feat: relaxed option naming (#627)
* add a test for std::map * add some test of the relaxed naming and other checks * add validator for aliases, group names and option groups * add extra tests and update readme * style: pre-commit.ci fixes * update the book chapters * fix codacy issue * Apply suggestions from code review Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Philip Top · GitHub
1 parent d17016e7
Showing 11 changed files with 132 additions and 60 deletions
README.md
book/chapters/flags.md
book/chapters/options.md
include/CLI/App.hpp
include/CLI/Option.hpp
include/CLI/StringTools.hpp
tests/AppTest.cpp
tests/HelpersTest.cpp
tests/OptionGroupTest.cpp
tests/OptionTypeTest.cpp
tests/SubcommandTest.cpp
@@ -263,7 +263,7 @@ App* subcom = app.add_subcommand(name, description);
 Option_group *app.add_option_group(name,description);
 ```
  
-An option name must start with a alphabetic character, underscore, a number, '?', or '@'. For long options, after the first character '.', and '-' are also valid characters. For the `add_flag*` functions '{' has special meaning. Names are given as a comma separated string, with the dash or dashes. An option or flag can have as many names as you want, and afterward, using `count`, you can use any of the names, with dashes as needed, to count the options. One of the names is allowed to be given without proceeding dash(es); if present the option is a positional option, and that name will be used on the help line for its positional form.
+An option name may start with any character except ('-', ' ', '\n', and '!') 🚧. For long options, after the first character all characters are allowed except ('=',':','{',' ', '\n')🚧. For the `add_flag*` functions '{' and '!' have special meaning which is why they are not allowed. Names are given as a comma separated string, with the dash or dashes. An option or flag can have as many names as you want, and afterward, using `count`, you can use any of the names, with dashes as needed, to count the options. One of the names is allowed to be given without proceeding dash(es); if present the option is a positional option, and that name will be used on the help line for its positional form.
  
 The `add_option_function<type>(...` function will typically require the template parameter be given unless a `std::function` object with an exact match is passed.  The type can be any type supported by the `add_option` function. The function should throw an error (`CLI::ConversionError` or `CLI::ValidationError` possibly) if the value is not valid.
  
@@ -557,7 +557,7 @@ even exit the program through the callback.
 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 unless the `.immediate_callback()`  flag is set or the callback is specified through the `parse_complete_callback()` function. The `final_callback()` is triggered only once.  In which case the callback executes on completion of the subcommand arguments but after the arguments for that subcommand have been parsed, and can be triggered multiple times.
  
 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.  Callbacks for nameless subcommands are only triggered if any options from the subcommand were parsed.
+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.  Callbacks for nameless subcommands are only triggered if any options from the subcommand were parsed. Subcommand names given through the `add_subcommand` method have the same restrictions as option names.
  
 #### Subcommand options
  
@@ -668,7 +668,7 @@ The subcommand method
 .add_option_group(name,description)
 ```
  
-Will create an option group, and return a pointer to it. The argument for `description` is optional and can be omitted.  An option group allows creation of a collection of options, similar to the groups function on options, but with additional controls and requirements.  They allow specific sets of options to be composed and controlled as a collective.  For an example see [range example](https://github.com/CLIUtils/CLI11/blob/master/examples/ranges.cpp).  Option groups are a specialization of an App so all [functions](#subcommand-options) that work with an App or subcommand also work on option groups.  Options can be created as part of an option group using the add functions just like a subcommand, or previously created options can be added through
+Will create an option group, and return a pointer to it. The argument for `description` is optional and can be omitted.  An option group allows creation of a collection of options, similar to the groups function on options, but with additional controls and requirements.  They allow specific sets of options to be composed and controlled as a collective.  For an example see [range example](https://github.com/CLIUtils/CLI11/blob/master/examples/ranges.cpp).  Option groups are a specialization of an App so all [functions](#subcommand-options) that work with an App or subcommand also work on option groups.  Options can be created as part of an option group using the add functions just like a subcommand, or previously created options can be added through.  The name given in an option group must not contain newlines or null characters.🚧
  
 ```cpp
 ogroup->add_option(option_pointer);
@@ -11,7 +11,7 @@ bool my_flag{false};
 app.add_flag("-f", my_flag, "Optional description");
 ```
  
-This will bind the flag `-f` to the boolean `my_flag`. After the parsing step, `my_flag` will be `false` if the flag was not found on the command line, or `true` if it was. By default, it will be allowed any number of times, but if you explicitly[^1] request `->take_last(false)`, it will only be allowed once; passing something like `./my_app -f -f` or `./my_app -ff` will throw a `ParseError` with a nice help description.
+This will bind the flag `-f` to the boolean `my_flag`. After the parsing step, `my_flag` will be `false` if the flag was not found on the command line, or `true` if it was. By default, it will be allowed any number of times, but if you explicitly\[^1\] request `->take_last(false)`, it will only be allowed once; passing something like `./my_app -f -f` or `./my_app -ff` will throw a `ParseError` with a nice help description. A flag name may start with any character except ('-', ' ', '\n', and '!'). For long flags, after the first character all characters are allowed except ('=',':','{',' ', '\n').  Names are given as a comma separated string, with the dash or dashes. An flag can have as many names as you want, and afterward, using `count`, you can use any of the names, with dashes as needed.
  
 ## Integer flags
  
@@ -120,4 +120,4 @@ Flag int: 3
 Flag plain: 1
 ```
  
-[^1]: It will not inherit this from the parent defaults, since this is often useful even if you don't want all options to allow multiple passed options.
+\[^1\]: It will not inherit this from the parent defaults, since this is often useful even if you don't want all options to allow multiple passed options.
@@ -26,12 +26,12 @@ You can use any C++ int-like type, not just `int`. CLI11 understands the followi
 | complex-number | std::complex or any type which has a real(), and imag() operations available, will allow 1 or 2 string definitions like "1+2j" or two arguments "1","2" |
 | enumeration | any enum or enum class type is supported through conversion from the underlying type(typically int, though it can be specified otherwise) |
 | container-like | a container(like vector) of any available types including other containers |
-| wrapper | any other object with a `value_type` static definition where the type specified by `value_type` is one of type in this list |
+| wrapper | any other object with a `value_type` static definition where the type specified by `value_type` is one of the type in this list, including `std::atomic<>` |
 | tuple | a tuple, pair, or array, or other type with a tuple size and tuple_type operations defined and the members being a type contained in this list |
 | function | A function that takes an array of strings and returns a string that describes the conversion failure or empty for success. May be the empty function. (`{}`) |
 | streamable | any other type with a `<<` operator will also work |
  
-By default, CLI11 will assume that an option is optional, and one value is expected if you do not use a vector. You can change this on a specific option using option modifiers.
+By default, CLI11 will assume that an option is optional, and one value is expected if you do not use a vector. You can change this on a specific option using option modifiers.  An option name may start with any character except ('-', ' ', '\n', and '!'). For long options, after the first character all characters are allowed except ('=',':','{',' ', '\n'). Names are given as a comma separated string, with the dash or dashes. An option can have as many names as you want, and afterward, using `count`, you can use any of the names, with dashes as needed, to count the options. One of the names is allowed to be given without proceeding dash(es); if present the option is a positional option, and that name will be used on the help line for its positional form.
  
 ## Positional options and aliases
  
@@ -282,4 +282,4 @@ There are some additional options that can be specified to modify an option for 
  
 ## Unusual circumstances
  
-There are a few cases where some things break down in the type system managing options and definitions.  Using the `add_option` method defines a lambda function to extract a default value if required.  In most cases this either straightforward or a failure is detected automatically and handled.  But in a few cases a streaming template is available that several layers down may not actually be defined.  The conditions in CLI11 cannot detect this circumstance automatically and will result in compile error.  One specific known case is `boost::optional` if the boost optional_io header is included.  This header defines a template for all boost optional values even if they do no actually have a streaming operator.  For example `boost::optional<std::vector>` does not have a streaming operator but one is detected since it is part of a template.  For these cases a secondary method `app->add_option_no_stream(...)` is provided that bypasses this operation completely and should compile in these cases.
+There are a few cases where some things break down in the type system managing options and definitions.  Using the `add_option` method defines a lambda function to extract a default value if required.  In most cases this is either straightforward or a failure is detected automatically and handled.  But in a few cases a streaming template is available that several layers down may not actually be defined.  This results in CLI11 not being able to detect this circumstance automatically and will result in compile error.  One specific known case is `boost::optional` if the boost optional_io header is included.  This header defines a template for all boost optional values even if they do not actually have a streaming operator.  For example `boost::optional<std::vector>` does not have a streaming operator but one is detected since it is part of a template.  For these cases a secondary method `app->add_option_no_stream(...)` is provided that bypasses this operation completely and should compile in these cases.
@@ -368,23 +368,9 @@ class App {
  
     /// Set an alias for the app
     App *alias(std::string app_name) {
-        if(!detail::valid_name_string(app_name)) {
-            if(app_name.empty()) {
-                throw IncorrectConstruction("Empty aliases are not allowed");
-            }
-            if(!detail::valid_first_char(app_name[0])) {
-                throw IncorrectConstruction(
-                    "Alias starts with invalid character, allowed characters are [a-zA-z0-9]+'_','?','@' ");
-            }
-            for(auto c : app_name) {
-                if(!detail::valid_later_char(c)) {
-                    throw IncorrectConstruction(std::string("Alias contains invalid character ('") + c +
-                                                "'), allowed characters are "
-                                                "[a-zA-z0-9]+'_','?','@','.','-' ");
-                }
-            }
+        if(app_name.empty() || !detail::valid_alias_name_string(app_name)) {
+            throw IncorrectConstruction("Aliases may not be empty or contain newlines or null characters");
         }
-
         if(parent_ != nullptr) {
             aliases_.push_back(app_name);
             auto &res = _compare_subcommand_names(*this, *_get_fallthrough_parent());
@@ -961,6 +947,9 @@ class App {
     /// creates an option group as part of the given app
     template <typename T = Option_group>
     T *add_option_group(std::string group_name, std::string group_description = "") {
+        if(!detail::valid_alias_name_string(group_name)) {
+            throw IncorrectConstruction("option group names may not contain newlines or null characters");
+        }
         auto option_group = std::make_shared<T>(std::move(group_description), group_name, this);
         auto ptr = option_group.get();
         // move to App_p for overload resolution on older gcc versions
@@ -978,13 +967,13 @@ class App {
         if(!subcommand_name.empty() && !detail::valid_name_string(subcommand_name)) {
             if(!detail::valid_first_char(subcommand_name[0])) {
                 throw IncorrectConstruction(
-                    "Subcommand name starts with invalid character, allowed characters are [a-zA-z0-9]+'_','?','@' ");
+                    "Subcommand name starts with invalid character, '!' and '-' are not allowed");
             }
             for(auto c : subcommand_name) {
                 if(!detail::valid_later_char(c)) {
                     throw IncorrectConstruction(std::string("Subcommand name contains invalid character ('") + c +
-                                                "'), allowed characters are "
-                                                "[a-zA-z0-9]+'_','?','@','.','-' ");
+                                                "'), all characters are allowed except"
+                                                "'=',':','{','}', and ' '");
                 }
             }
         }
@@ -94,6 +94,9 @@ template &lt;typename CRTP&gt; class OptionBase {
  
     /// Changes the group membership
     CRTP *group(const std::string &name) {
+        if(!detail::valid_alias_name_string(name)) {
+            throw IncorrectConstruction("Group names may not contain newlines or null characters");
+        }
         group_ = name;
         return static_cast<CRTP *>(this);
     }
@@ -157,6 +157,22 @@ inline std::string &amp;remove_quotes(std::string &amp;str) {
     return str;
 }
  
+/// Add a leader to the beginning of all new lines (nothing is added
+/// at the start of the first line). `"; "` would be for ini files
+///
+/// Can't use Regex, or this would be a subs.
+inline std::string fix_newlines(const std::string &leader, std::string input) {
+    std::string::size_type n = 0;
+    while(n != std::string::npos && n < input.size()) {
+        n = input.find('\n', n);
+        if(n != std::string::npos) {
+            input = input.substr(0, n + 1) + leader + input.substr(n + 1);
+            n += leader.size();
+        }
+    }
+    return input;
+}
+
 /// Make a copy of the string and then trim it, any filter string can be used (any char in string is filtered)
 inline std::string trim_copy(const std::string &str, const std::string &filter) {
     std::string s = str;
@@ -191,7 +207,7 @@ inline std::ostream &amp;format_aliases(std::ostream &amp;out, const std::vector&lt;std::st
             } else {
                 front = false;
             }
-            out << alias;
+            out << detail::fix_newlines("              ", alias);
         }
         out << "\n";
     }
@@ -199,23 +215,35 @@ inline std::ostream &amp;format_aliases(std::ostream &amp;out, const std::vector&lt;std::st
 }
  
 /// Verify the first character of an option
-template <typename T> bool valid_first_char(T c) {
-    return std::isalnum(c, std::locale()) || c == '_' || c == '?' || c == '@';
-}
+/// - is a trigger character, ! has special meaning and new lines would just be annoying to deal with
+template <typename T> bool valid_first_char(T c) { return ((c != '-') && (c != '!') && (c != ' ') && c != '\n'); }
  
 /// Verify following characters of an option
-template <typename T> bool valid_later_char(T c) { return valid_first_char(c) || c == '.' || c == '-'; }
+template <typename T> bool valid_later_char(T c) {
+    // = and : are value separators, { has special meaning for option defaults,
+    // and \n would just be annoying to deal with in many places allowing space here has too much potential for
+    // inadvertent entry errors and bugs
+    return ((c != '=') && (c != ':') && (c != '{') && (c != ' ') && c != '\n');
+}
  
-/// Verify an option name
+/// Verify an option/subcommand name
 inline bool valid_name_string(const std::string &str) {
-    if(str.empty() || !valid_first_char(str[0]))
+    if(str.empty() || !valid_first_char(str[0])) {
         return false;
-    for(auto c : str.substr(1))
-        if(!valid_later_char(c))
+    }
+    auto e = str.end();
+    for(auto c = str.begin() + 1; c != e; ++c)
+        if(!valid_later_char(*c))
             return false;
     return true;
 }
  
+/// Verify an app name
+inline bool valid_alias_name_string(const std::string &str) {
+    static const std::string badChars(std::string("\n") + '\0');
+    return (str.find_first_of(badChars) == std::string::npos);
+}
+
 /// check if a string is a container segment separator (empty or "%%")
 inline bool is_separator(const std::string &str) {
     static const std::string sep("%%");
@@ -260,7 +288,7 @@ inline bool has_default_flag_values(const std::string &amp;flags) {
 }
  
 inline void remove_default_flag_values(std::string &flags) {
-    auto loc = flags.find_first_of('{');
+    auto loc = flags.find_first_of('{', 2);
     while(loc != std::string::npos) {
         auto finish = flags.find_first_of("},", loc + 1);
         if((finish != std::string::npos) && (flags[finish] == '}')) {
@@ -367,22 +395,6 @@ inline std::vector&lt;std::string&gt; split_up(std::string str, char delimiter = &#39;\0&#39;)
     return output;
 }
  
-/// Add a leader to the beginning of all new lines (nothing is added
-/// at the start of the first line). `"; "` would be for ini files
-///
-/// Can't use Regex, or this would be a subs.
-inline std::string fix_newlines(const std::string &leader, std::string input) {
-    std::string::size_type n = 0;
-    while(n != std::string::npos && n < input.size()) {
-        n = input.find('\n', n);
-        if(n != std::string::npos) {
-            input = input.substr(0, n + 1) + leader + input.substr(n + 1);
-            n += leader.size();
-        }
-    }
-    return input;
-}
-
 /// This function detects an equal or colon followed by an escaped quote after an argument
 /// then modifies the string to replace the equality with a space.  This is needed
 /// to allow the split up function to work properly and is intended to be used with the find_and_modify function
@@ -127,6 +127,17 @@ TEST_CASE_METHOD(TApp, &quot;DashedOptionsSingleString&quot;, &quot;[app]&quot;) {
     CHECK(app.count("--that") == 2u);
 }
  
+TEST_CASE_METHOD(TApp, "StrangeFlagNames", "[app]") {
+    app.add_flag("-=");
+    app.add_flag("--t\tt");
+    app.add_flag("-{");
+    CHECK_THROWS_AS(app.add_flag("--t t"), CLI::ConstructionError);
+    args = {"-=", "--t\tt"};
+    run();
+    CHECK(app.count("-=") == 1u);
+    CHECK(app.count("--t\tt") == 1u);
+}
+
 TEST_CASE_METHOD(TApp, "RequireOptionsError", "[app]") {
     using Catch::Matchers::Contains;
  
@@ -582,6 +593,20 @@ TEST_CASE_METHOD(TApp, &quot;SingleArgVector&quot;, &quot;[app]&quot;) {
     CHECK("happy" == path);
 }
  
+TEST_CASE_METHOD(TApp, "StrangeOptionNames", "[app]") {
+    app.add_option("-:");
+    app.add_option("--t\tt");
+    app.add_option("--{}");
+    app.add_option("--:)");
+    CHECK_THROWS_AS(app.add_option("--t t"), CLI::ConstructionError);
+    args = {"-:)", "--{}", "5"};
+    run();
+    CHECK(app.count("-:") == 1u);
+    CHECK(app.count("--{}") == 1u);
+    CHECK(app["-:"]->as<char>() == ')');
+    CHECK(app["--{}"]->as<int>() == 5);
+}
+
 TEST_CASE_METHOD(TApp, "FlagLikeOption", "[app]") {
     bool val{false};
     auto opt = app.add_option("--flag", val)->type_size(0)->default_str("true");
@@ -155,13 +155,14 @@ TEST_CASE(&quot;String: InvalidName&quot;, &quot;[helpers]&quot;) {
     CHECK(CLI::detail::valid_name_string("valid"));
     CHECK_FALSE(CLI::detail::valid_name_string("-invalid"));
     CHECK(CLI::detail::valid_name_string("va-li-d"));
-    CHECK_FALSE(CLI::detail::valid_name_string("vali&d"));
+    CHECK_FALSE(CLI::detail::valid_name_string("valid{}"));
     CHECK(CLI::detail::valid_name_string("_valid"));
-    CHECK_FALSE(CLI::detail::valid_name_string("/valid"));
+    CHECK(CLI::detail::valid_name_string("/valid"));
     CHECK(CLI::detail::valid_name_string("vali?d"));
     CHECK(CLI::detail::valid_name_string("@@@@"));
     CHECK(CLI::detail::valid_name_string("b@d2?"));
     CHECK(CLI::detail::valid_name_string("2vali?d"));
+    CHECK_FALSE(CLI::detail::valid_name_string("!valid"));
 }
  
 TEST_CASE("StringTools: Modify", "[helpers]") {
@@ -23,6 +23,16 @@ TEST_CASE_METHOD(TApp, &quot;BasicOptionGroup&quot;, &quot;[optiongroup]&quot;) {
     CHECK(1u == app.count_all());
 }
  
+TEST_CASE_METHOD(TApp, "OptionGroupInvalidNames", "[optiongroup]") {
+    CHECK_THROWS_AS(app.add_option_group("clusters\ncluster2", "description"), CLI::IncorrectConstruction);
+
+    std::string groupName("group1");
+    groupName += '\0';
+    groupName.append("group2");
+
+    CHECK_THROWS_AS(app.add_option_group(groupName), CLI::IncorrectConstruction);
+}
+
 TEST_CASE_METHOD(TApp, "BasicOptionGroupExact", "[optiongroup]") {
     auto ogroup = app.add_option_group("clusters");
     int res{0};
@@ -269,6 +269,17 @@ TEST_CASE_METHOD(TApp, &quot;vectorDefaults&quot;, &quot;[optiontype]&quot;) {
     CHECK(std::vector<int>({5}) == res);
 }
  
+TEST_CASE_METHOD(TApp, "mapInput", "[optiontype]") {
+    std::map<int, std::string> vals{};
+    app.add_option("--long", vals);
+
+    args = {"--long", "5", "test"};
+
+    run();
+
+    CHECK(vals.at(5) == "test");
+}
+
 TEST_CASE_METHOD(TApp, "CallbackBoolFlags", "[optiontype]") {
  
     bool value{false};
@@ -815,10 +815,10 @@ TEST_CASE_METHOD(TApp, &quot;invalidSubcommandName&quot;, &quot;[subcom]&quot;) {
  
     bool gotError{false};
     try {
-        app.add_subcommand("foo/foo", "Foo a bar");
+        app.add_subcommand("!foo/foo", "Foo a bar");
     } catch(const CLI::IncorrectConstruction &e) {
         gotError = true;
-        CHECK_THAT(e.what(), Contains("/"));
+        CHECK_THAT(e.what(), Contains("!"));
     }
     CHECK(gotError);
 }
@@ -1645,6 +1645,28 @@ TEST_CASE_METHOD(TApp, &quot;OptionGroupAlias&quot;, &quot;[subcom]&quot;) {
     CHECK(-3 == val);
 }
  
+TEST_CASE_METHOD(TApp, "OptionGroupAliasWithSpaces", "[subcom]") {
+    double val{0.0};
+    auto sub = app.add_option_group("sub1");
+    sub->alias("sub2 bb");
+    sub->alias("sub3/b");
+    sub->add_option("-v,--value", val);
+    args = {"sub1", "-v", "-3"};
+    CHECK_THROWS_AS(run(), CLI::ExtrasError);
+
+    args = {"sub2 bb", "--value", "-5"};
+    run();
+    CHECK(-5.0 == val);
+
+    args = {"sub3/b", "-v", "7"};
+    run();
+    CHECK(7 == val);
+
+    args = {"-v", "-3"};
+    run();
+    CHECK(-3 == val);
+}
+
 TEST_CASE_METHOD(TApp, "subcommand_help", "[subcom]") {
     auto sub1 = app.add_subcommand("help")->silent();
     bool flag{false};
@@ -1666,9 +1688,8 @@ TEST_CASE_METHOD(TApp, &quot;AliasErrors&quot;, &quot;[subcom]&quot;) {
     auto sub1 = app.add_subcommand("sub1");
     auto sub2 = app.add_subcommand("sub2");
  
-    CHECK_THROWS_AS(sub2->alias("this is a not a valid alias"), CLI::IncorrectConstruction);
-    CHECK_THROWS_AS(sub2->alias("-alias"), CLI::IncorrectConstruction);
-    CHECK_THROWS_AS(sub2->alias("alia$"), CLI::IncorrectConstruction);
+    CHECK_THROWS_AS(sub2->alias("this is a not\n a valid alias"), CLI::IncorrectConstruction);
+    CHECK_NOTHROW(sub2->alias("-alias"));  // this is allowed but would be unusable on command line parsers
  
     CHECK_THROWS_AS(app.add_subcommand("--bad_subcommand_name", "documenting the bad subcommand"),
                     CLI::IncorrectConstruction);