make immediate_callback inheritable, add tests for that and for valid strings, s…

…ome cleanup in the README.md Apply suggestions from code review Co-Authored-By: phlptp <top1@llnl.gov> allow callbacks for option_groups, and allow some other characters as flags

make immediate_callback inheritable, add tests for that and for valid strings, s…
…ome cleanup in the README.md Apply suggestions from code review Co-Authored-By: phlptp <top1@llnl.gov> allow callbacks for option_groups, and allow some other characters as flags
Philip Top · Henry Schreiner
1 parent 6ad2641d
Showing 9 changed files with 125 additions and 37 deletions
README.md
include/CLI/App.hpp
include/CLI/Option.hpp
include/CLI/StringTools.hpp
tests/CreationTest.cpp
tests/HelpTest.cpp
tests/HelpersTest.cpp
tests/OptionGroupTest.cpp
tests/SubcommandTest.cpp
@@ -236,7 +236,7 @@ Option_group *app.add_option_group(name,description); // 🚧
 -app.add_mutable_set_ignore_case_underscore(... // 🆕 String only
 ```
-An option name must start with a alphabetic character, underscore, or a number🚧. For long options, after the first character '.', and '-' are also valid. 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 help line for its positional form. If you want the default value to print in the help description, pass in `true` for the final parameter for `add_option`.
+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 help line for its positional form. If you want the default value to print in the help description, pass in `true` for the final parameter for `add_option`.
 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.
@@ -511,9 +511,10 @@ There are several options that are supported on the main app and subcommands and
 #### Callbacks 
 A subcommand has two optional callbacks that are executed at different stages of processing.  The `preparse_callback` 🚧 is executed once after the first argument of a subcommand or application is processed and gives an argument for the number of remaining arguments to process.  For the main app the first argument is considered the program name,  for subcommands the first argument is the subcommand name.  For Option groups and nameless subcommands the first argument is after the first argument or subcommand is processed from that group.
-The second callback is executed after parsing.  Depending on the status of the `immediate_callback` flag 🚧.  This is either immediately after the parsing of the subcommand.  Or if the flag is false, once after parsing of all arguments.  If the immediate_callback is set then the callback can be executed multiple times.   If the main app or subcommand has a config file, no data from the config file will be reflected in immediate_callback.
+The second callback is executed after parsing.  The behavior depends on the status of the `immediate_callback` flag 🚧. If true, this runs immediately after the parsing of the subcommand.  Or if the flag is false, once after parsing of all arguments.  If the `immediate_callback` is set then the callback can be executed multiple times if the subcommand list given multiple times.  If the main app or subcommand has a config file, no data from the config file will be reflected in immediate_callback. `immediate_callback()` has no effect on the main app, though it can be inherited.  For option_groups `immediate_callback` causes the callback to be run prior to other option groups and options in the main app, effectively giving the options in the group priority.  
 For example say an application was set up like
+
 ```cpp
 app.callback(ac);
 sub1=app.add_subcommand("sub1")->callback(c1)->preparse_callback(pc1)->immediate_callback();
@@ -523,6 +524,7 @@ app.preparse_callback( pa1);
 ... A bunch of other options
 ```
+
 Then the command line is given as
 ```
@@ -534,8 +536,8 @@ program --opt1 opt1_val  sub1 --sub1opt --sub1optb val sub2 --sub2opt sub1 --sub
 * c1 will be called when the `sub2` command is encountered
 * pc2 will be called with value of 6 after the sub2 command is encountered.
 * c1 will be called again after the second sub2 command is encountered
-* ac will be called after completing the parse
 * c2 will be called once after processing all arguments 
+* ac will be called after completing the parse and all lower level callbacks have been executed
 A subcommand is considered terminated when one of the following conditions are met.
 1. There are no more arguments to process
@@ -549,20 +551,26 @@ If the `immediate_callback` flag is set then all contained options are processed
 #### Option groups 🚧
-The subcommand method 
+The subcommand method
+
 ```cpp
 .add_option_group(name,description)
 ```
+
 Will create an option Group, and return a pointer to it.  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 test](./tests/ranges.cpp).  Option groups are a specialization of an App so all [functions](#subcommand-options) that work with an App 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
+
 ```cpp
 ogroup->add_option(option_pointer)
 ```
+
 ```cpp
 ogroup->add_options(option_pointer)
 ```
+
 ```cpp
 ogroup->add_options(option1,option2,option3,...)
 ```
+
 The option pointers used in this function must be options defined in the parent application of the option group otherwise an error will be generated.  
 Options in an option group are searched for a command line match after any options in the main app, so any positionals in the main app would be matched first.  So care must be taken to make sure of the order when using positional arguments and option groups.
 Option groups work well with `excludes` and `require_options` methods, as an Application will treat an option group as a single option for the purpose of counting and requirements.  Option groups allow specifying requirements such as requiring 1 of 3 options in one group and 1 of 3 options in a different group. Option groups can contain other groups as well.   Disabling an option group will turn off all options within the group.  
@@ -601,7 +609,7 @@ arguments, use `.config_to_str(default_also=false, prefix=&quot;&quot;, write_description=
 ### Inheriting defaults
-Many of the defaults for subcommands and even options are inherited from their creators. The inherited default values for subcommands are `allow_extras`, `prefix_command`, `ignore_case`, 🆕 `ignore_underscore`, `fallthrough`, `group`, `footer`, and maximum number of required subcommands. The help flag existence, name, and description are inherited, as well.
+Many of the defaults for subcommands and even options are inherited from their creators. The inherited default values for subcommands are `allow_extras`, `prefix_command`, `ignore_case`, 🆕 `ignore_underscore`, `fallthrough`, `group`, `footer`,`immediate_callback` and maximum number of required subcommands. The help flag existence, name, and description are inherited, as well.
 Options have defaults for `group`, `required`, `multi_option_policy`, `ignore_case`, 🆕 `ignore_underscore`, 🚧 `delimiter`, and 🚧 `disable_flag_override`. To set these defaults, you should set the `option_defaults()` object, for example:
@@ -94,7 +94,7 @@ class App {
     bool pre_parse_called_{false};
     /// Flag indicating that the callback for the subcommand should be executed immediately on parse completion which is
-    /// before help or ini files are processed.
+    /// before help or ini files are processed. INHERITABLE
     bool immediate_callback_{false};
     /// This is a function that runs prior to the start of parsing
@@ -246,6 +246,7 @@ class App {
             allow_extras_ = parent_->allow_extras_;
             allow_config_extras_ = parent_->allow_config_extras_;
             prefix_command_ = parent_->prefix_command_;
+            immediate_callback_ = parent_->immediate_callback_;
             ignore_case_ = parent_->ignore_case_;
             ignore_underscore_ = parent_->ignore_underscore_;
             fallthrough_ = parent_->fallthrough_;
@@ -320,7 +321,7 @@ class App {
         return this;
     }
-    /// Set the subcommand to be enabled by default, so on clear(), at the start of each parse it is enabled(not
+    /// Set the subcommand to be enabled by default, so on clear(), at the start of each parse it is enabled (not
     /// disabled)
     App *enabled_by_default(bool enable = true) {
         enabled_by_default_ = enable;
@@ -1778,15 +1779,26 @@ class App {
             app->_configure();
         }
     }
-    /// Internal function to run (App) callback, top down
+    /// Internal function to run (App) callback, bottom up
     void run_callback() {
         pre_callback();
-        if(callback_ && (parsed_ > 0))
-            callback_();
+        // run the callbacks for the received subcommands
         for(App *subc : get_subcommands()) {
-            if((subc->get_name().empty()) || (!subc->immediate_callback_))
+            if(!subc->immediate_callback_)
                 subc->run_callback();
         }
+        // now run callbacks for option_groups
+        for(auto &subc : subcommands_) {
+            if(!subc->immediate_callback_ && subc->name_.empty() && subc->count_all() > 0) {
+                subc->run_callback();
+            }
+        }
+        // finally run the main callback
+        if(callback_ && (parsed_ > 0)) {
+            if(!name_.empty() || count_all() > 0) {
+                callback_();
+            }
+        }
     }
     /// Check to see if a subcommand is valid. Give up immediately if subcommand max has been reached.
@@ -1817,7 +1829,7 @@ class App {
             return detail::Classifier::SHORT;
         if((allow_windows_style_options_) && (detail::split_windows_style(current, dummy1, dummy2)))
             return detail::Classifier::WINDOWS;
-        if((current == "++") && (!name_.empty()))
+        if((current == "++") && !name_.empty())
             return detail::Classifier::SUBCOMMAND_TERMINATOR;
         return detail::Classifier::NONE;
     }
@@ -1872,13 +1884,24 @@ class App {
         }
         for(App_p &sub : subcommands_) {
-            if((sub->get_name().empty()) || (!sub->immediate_callback_))
+            if(sub->get_name().empty() || !sub->immediate_callback_)
                 sub->_process_env();
         }
     }
     /// Process callbacks. Runs on *all* subcommands.
     void _process_callbacks() {
+
+        for(App_p &sub : subcommands_) {
+            // process the priority option_groups first
+            if(sub->get_name().empty() && sub->immediate_callback_) {
+                if(sub->count_all() > 0) {
+                    sub->_process_callbacks();
+                    sub->run_callback();
+                }
+            }
+        }
+
         for(const Option_p &opt : options_) {
             if(opt->count() > 0 && !opt->get_callback_run()) {
                 opt->run_callback();
@@ -1886,8 +1909,9 @@ class App {
         }
         for(App_p &sub : subcommands_) {
-            if((sub->get_name().empty()) || (!sub->immediate_callback_))
+            if(!sub->immediate_callback_) {
                 sub->_process_callbacks();
+            }
         }
     }
@@ -1979,13 +2003,12 @@ class App {
         for(App_p &sub : subcommands_) {
             if(sub->disabled_)
                 continue;
-            if((sub->name_.empty()) && (sub->count_all() > 0)) {
+            if(sub->name_.empty() && sub->count_all() > 0) {
                 ++used_options;
             }
         }
-        if((require_option_min_ > used_options) ||
-           ((require_option_max_ > 0) && (require_option_max_ < used_options))) {
+        if(require_option_min_ > used_options || (require_option_max_ > 0 && require_option_max_ < used_options)) {
             auto option_list = detail::join(options_, [](const Option_p &ptr) { return ptr->get_name(false, true); });
             if(option_list.compare(0, 10, "-h,--help,") == 0) {
                 option_list.erase(0, 10);
@@ -2001,25 +2024,25 @@ class App {
         for(App_p &sub : subcommands_) {
             if(sub->disabled_)
                 continue;
-            if((sub->name_.empty()) && (sub->required_ == false)) {
+            if(sub->name_.empty() && sub->required_ == false) {
                 if(sub->count_all() == 0) {
-                    if((require_option_min_ > 0) && (require_option_min_ <= used_options)) {
+                    if(require_option_min_ > 0 && require_option_min_ <= used_options) {
                         continue;
                         // if we have met the requirement and there is nothing in this option group skip checking
                         // requirements
                     }
-                    if((require_option_max_ > 0) && (used_options >= require_option_min_)) {
+                    if(require_option_max_ > 0 && used_options >= require_option_min_) {
                         continue;
                         // if we have met the requirement and there is nothing in this option group skip checking
                         // requirements
                     }
                 }
             }
-            if((sub->count() > 0) || (sub->name_.empty())) {
+            if(sub->count() > 0 || sub->name_.empty()) {
                 sub->_process_requirements();
             }
-            if((sub->required_) && (sub->count_all() == 0)) {
+            if(sub->required_ && sub->count_all() == 0) {
                 throw(CLI::RequiredError(sub->get_display_name()));
             }
         }
@@ -2225,14 +2248,13 @@ class App {
                 }
             }
         }
-        /// let the parent deal with it if possible
+        // let the parent deal with it if possible
         if(parent_ != nullptr && fallthrough_)
             return _get_fallthrough_parent()->_parse_positional(args);
         /// 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(com != nullptr && (require_subcommand_max_ == 0 || require_subcommand_max_ > parsed_subcommands_.size())) {
             args.pop_back();
             com->_parse(args);
             return true;
@@ -2241,8 +2263,8 @@ class App {
         /// subcommand in a broader way, if one exists let the parent deal with it
         auto parent_app = (parent_ != nullptr) ? _get_fallthrough_parent() : this;
         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()))) {
+        if(com != nullptr && (com->parent_->require_subcommand_max_ == 0 ||
+                              com->parent_->require_subcommand_max_ > com->parent_->parsed_subcommands_.size())) {
             return false;
         }
@@ -2250,7 +2272,7 @@ class App {
             throw CLI::ExtrasError(args);
         }
         /// If this is an option group don't deal with it
-        if((parent_ != nullptr) && (name_.empty())) {
+        if(parent_ != nullptr && name_.empty()) {
             return false;
         }
         /// We are out of other options this goes to missing
@@ -2270,7 +2292,7 @@ class App {
     /// 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))
+            if(com->disabled_ && ignore_disabled)
                 continue;
             if(com->get_name().empty()) {
                 auto subc = com->_find_subcommand(subc_name, ignore_disabled, ignore_used);
@@ -2278,7 +2300,7 @@ class App {
                     return subc;
                 }
             } else if(com->check_name(subc_name)) {
-                if((!*com) || (!ignore_used))
+                if((!*com) || !ignore_used)
                     return com.get();
             }
         }
@@ -2353,7 +2375,7 @@ class App {
         // Option not found
         if(op_ptr == std::end(options_)) {
             for(auto &subc : subcommands_) {
-                if((subc->name_.empty()) && (!(subc->disabled_))) {
+                if(subc->name_.empty() && !subc->disabled_) {
                     if(subc->_parse_arg(args, current_type)) {
                         if(!subc->pre_parse_called_) {
                             subc->_trigger_pre_parse(args.size());
@@ -2366,7 +2388,7 @@ class App {
             if(parent_ != nullptr && fallthrough_)
                 return _get_fallthrough_parent()->_parse_arg(args, current_type);
             // don't capture missing if this is a nameless subcommand
-            if((parent_ != nullptr) && (name_.empty())) {
+            if(parent_ != nullptr && name_.empty()) {
                 return false;
             }
             // Otherwise, add to missing
@@ -667,7 +667,7 @@ class Option : public OptionBase&lt;Option&gt; {
         // Run the validators (can change the string)
         if(!validators_.empty()) {
             for(std::string &result : results_)
-                for(const std::function<std::string(std::string &)> &vali : validators_) {
+                for(const auto &vali : validators_) {
                     std::string err_msg;
                     try {
@@ -171,12 +171,12 @@ inline std::ostream &amp;format_help(std::ostream &amp;out, std::string name, std::strin
 }
 /// Verify the first character of an option
-template <typename T> bool valid_first_char(T c) { return std::isalnum(c, std::locale()) || c == '_'; }
+template <typename T> bool valid_first_char(T c) {
+    return std::isalnum(c, std::locale()) || c == '_' || c == '?' || c == '@';
+}
 /// Verify following characters of an option
-template <typename T> bool valid_later_char(T c) {
-    return std::isalnum(c, std::locale()) || c == '_' || c == '.' || c == '-';
-}
+template <typename T> bool valid_later_char(T c) { return valid_first_char(c) || c == '.' || c == '-'; }
 /// Verify an option name
 inline bool valid_name_string(const std::string &str) {
@@ -472,6 +472,7 @@ TEST_F(TApp, SubcommandDefaults) {
     // Initial defaults
     EXPECT_FALSE(app.get_allow_extras());
     EXPECT_FALSE(app.get_prefix_command());
+    EXPECT_FALSE(app.get_immediate_callback());
     EXPECT_FALSE(app.get_ignore_case());
     EXPECT_FALSE(app.get_ignore_underscore());
 #ifdef _WIN32
@@ -487,6 +488,7 @@ TEST_F(TApp, SubcommandDefaults) {
     app.allow_extras();
     app.prefix_command();
+    app.immediate_callback();
     app.ignore_case();
     app.ignore_underscore();
 #ifdef _WIN32
@@ -505,6 +507,7 @@ TEST_F(TApp, SubcommandDefaults) {
     // Initial defaults
     EXPECT_TRUE(app2->get_allow_extras());
     EXPECT_TRUE(app2->get_prefix_command());
+    EXPECT_TRUE(app2->get_immediate_callback());
     EXPECT_TRUE(app2->get_ignore_case());
     EXPECT_TRUE(app2->get_ignore_underscore());
 #ifdef _WIN32
@@ -337,6 +337,17 @@ TEST(THelp, OnlyOneHelp) {
     EXPECT_THROW(app.parse(input), CLI::ExtrasError);
 }
+TEST(THelp, MultiHelp) {
+    CLI::App app{"My prog"};
+
+    // It is not supported to have more than one help flag, last one wins
+    app.set_help_flag("--help,-h,-?", "No short name allowed");
+    app.allow_windows_style_options();
+
+    std::vector<std::string> input{"/?"};
+    EXPECT_THROW(app.parse(input), CLI::CallForHelp);
+}
+
 TEST(THelp, OnlyOneAllHelp) {
     CLI::App app{"My prog"};
@@ -33,6 +33,10 @@ TEST(String, InvalidName) {
     EXPECT_FALSE(CLI::detail::valid_name_string("vali&d"));
     EXPECT_TRUE(CLI::detail::valid_name_string("_valid"));
     EXPECT_FALSE(CLI::detail::valid_name_string("/valid"));
+    EXPECT_TRUE(CLI::detail::valid_name_string("vali?d"));
+    EXPECT_TRUE(CLI::detail::valid_name_string("@@@@"));
+    EXPECT_TRUE(CLI::detail::valid_name_string("b@d2?"));
+    EXPECT_TRUE(CLI::detail::valid_name_string("2vali?d"));
 }
 TEST(StringTools, Modify) {
@@ -550,6 +550,30 @@ TEST_F(ManyGroups, SameSubcommand) {
     EXPECT_EQ(subs[1], sub2);
     EXPECT_EQ(subs[2], sub3);
 }
+TEST_F(ManyGroups, CallbackOrder) {
+    // only 1 group can be used
+    remove_required();
+    std::vector<int> callback_order;
+    g1->callback([&callback_order]() { callback_order.push_back(1); });
+    g2->callback([&callback_order]() { callback_order.push_back(2); });
+    main->callback([&callback_order]() { callback_order.push_back(3); });
+
+    args = {"--name2", "test"};
+    run();
+    EXPECT_EQ(callback_order, std::vector<int>({2, 3}));
+
+    callback_order.clear();
+    args = {"--name1", "t2", "--name2", "test"};
+    g2->immediate_callback();
+    run();
+    EXPECT_EQ(callback_order, std::vector<int>({2, 1, 3}));
+    callback_order.clear();
+
+    args = {"--name2", "test", "--name1", "t2"};
+    g2->immediate_callback(false);
+    run();
+    EXPECT_EQ(callback_order, std::vector<int>({1, 2, 3}));
+}
 struct ManyGroupsPreTrigger : public ManyGroups {
     size_t triggerMain, trigger1{87u}, trigger2{34u}, trigger3{27u};
@@ -908,6 +908,22 @@ TEST_F(SubcommandProgram, CallbackOrder) {
     EXPECT_EQ(callback_order, std::vector<int>({2, 1}));
 }
+TEST_F(SubcommandProgram, CallbackOrderImmediate) {
+    std::vector<int> callback_order;
+    start->callback([&callback_order]() { callback_order.push_back(1); })->immediate_callback();
+    stop->callback([&callback_order]() { callback_order.push_back(2); });
+
+    args = {"start", "stop", "start"};
+    run();
+    EXPECT_EQ(callback_order, std::vector<int>({1, 1, 2}));
+
+    callback_order.clear();
+
+    args = {"stop", "start", "stop", "start"};
+    run();
+    EXPECT_EQ(callback_order, std::vector<int>({1, 1, 2}));
+}
+
 struct ManySubcommands : public TApp {
     CLI::App *sub1;