Adding new parse layout (#178)

* Adding new parse layout

* Dropping shortcurcuit from help, since it has special override

* Refactor help call

* Dropping shortcurcuit since it is not needed now that help has custom behavoir

* Dropping MaxSubcommand error (cannot occur)

CHANGELOG.md

@@ -1,3 +1,14 @@
+## Version 1.7.0: Parse breakup
+
+The parsing procedure now maps much more sensibly to complex, nested subcommand structures. Each phase of the parsing happens on all subcommands before moving on with the next phase of the parse. This allows several features, like required environment variables, to work properly even through subcommand boundaries.
+Passing the same subcommand multiple times is better supported.
+
+* Subcommands now track how many times they were parsed in a parsing process. `count()` with no arguments will return the number of times a subcommand was encountered.
+* Parsing is now done in phases: `shortcurcuits`, `ini`, `env`, `callbacks`, and `requirements`; all subcommands complete a phase before moving on.
+* Calling parse multiple times is now officially supported without `clear` (automatic).
+* Dropped the mostly undocumented `short_curcuit` property, as help flag parsing is a bit more complex, and the default callback behavior of options now works properly.
+
+
 ## Version 1.6.2: Help-all
 
 This version fixes some formatting bugs with help-all. It also adds fixes for several warnings, including an experimental optional error on Clang 7. Several smaller fixes.

include/CLI/App.hpp

@@ -145,8 +145,8 @@ class App {
     /// A pointer to the parent if this is a subcommand
     App *parent_{nullptr};
 
-    /// True if this command/subcommand was parsed
-    bool parsed_{false};
+    /// Counts the number of times this command/subcommand was parsed
+    size_t parsed_ = 0;
 
     /// Minimum required subcommands (not inheritable!)
     size_t require_subcommand_min_ = 0;
@@ -284,7 +284,7 @@ class App {
     }
 
     /// Check to see if this subcommand was parsed, true only if received on command line.
-    bool parsed() const { return parsed_; }
+    bool parsed() const { return parsed_ > 0; }
 
     /// Get the OptionDefault object, to set option defaults
     OptionDefaults *option_defaults() { return &option_defaults_; }
@@ -408,8 +408,7 @@ class App {
 
         // Empty name will simply remove the help flag
         if(!name.empty()) {
-            help_ptr_ = add_flag_function(name, [](size_t) -> void { throw CallForHelp(); }, description);
-            help_ptr_->short_circuit(true);
+            help_ptr_ = add_flag(name, description);
             help_ptr_->configurable(false);
         }
 
@@ -425,8 +424,7 @@ class App {
 
         // Empty name will simply remove the help all flag
         if(!name.empty()) {
-            help_all_ptr_ = add_flag_function(name, [](size_t) -> void { throw CallForAllHelp(); }, description);
-            help_all_ptr_->short_circuit(true);
+            help_all_ptr_ = add_flag(name, description);
             help_all_ptr_->configurable(false);
         }
 
@@ -826,6 +824,10 @@ class App {
         throw OptionNotFound(subcom);
     }
 
+    /// No argument version of count counts the number of times this subcommand was
+    /// passed in. The main app will return 1.
+    size_t count() const { return parsed_; }
+
     /// Changes the group membership
     App *group(std::string name) {
         group_ = name;
@@ -870,7 +872,7 @@ class App {
 
     /// Check to see if this subcommand was parsed, true only if received on command line.
     /// This allows the subcommand to be directly checked.
-    operator bool() const { return parsed_; }
+    operator bool() const { return parsed_ > 0; }
 
     ///@}
     /// @name Extras for subclassing
@@ -917,15 +919,16 @@ class App {
     /// Changes the vector to the remaining options.
     void parse(std::vector<std::string> &args) {
         // Clear if parsed
-        if(parsed_)
+        if(parsed_ > 0)
             clear();
 
-        // Redundant (set by _parse on commands/subcommands)
+        // _parse is incremented in commands/subcommands,
         // but placed here to make sure this is cleared when
         // running parse after an error is thrown, even by _validate.
-        parsed_ = true;
-
+        parsed_ = 1;
         _validate();
+        parsed_ = 0;
+
         _parse(args);
         run_callback();
     }
@@ -1016,11 +1019,11 @@ class App {
     /// Check to see if given subcommand was selected
     bool got_subcommand(App *subcom) const {
         // get subcom needed to verify that this was a real subcommand
-        return get_subcommand(subcom)->parsed_;
+        return get_subcommand(subcom)->parsed_ > 0;
     }
 
     /// Check with name instead of pointer to see if subcommand was selected
-    bool got_subcommand(std::string name) const { return get_subcommand(name)->parsed_; }
+    bool got_subcommand(std::string name) const { return get_subcommand(name)->parsed_ > 0; }
 
     ///@}
     /// @name Help
@@ -1282,19 +1285,10 @@ class App {
         return detail::Classifer::NONE;
     }
 
-    /// Internal parse function
-    void _parse(std::vector<std::string> &args) {
-        parsed_ = true;
-        bool positional_only = false;
-
-        while(!args.empty()) {
-            _parse_single(args, positional_only);
-        }
-
-        for(const Option_p &opt : options_)
-            if(opt->get_short_circuit() && opt->count() > 0)
-                opt->run_callback();
+    // The parse function is now broken into several parts, and part of process
 
+    /// Read and process an ini file (main app only)
+    void _process_ini() {
         // Process an INI file
         if(config_ptr_ != nullptr) {
             if(*config_ptr_) {
@@ -1311,8 +1305,10 @@ class App {
                 }
             }
         }
+    }
 
-        // Get envname options if not yet passed
+    /// Get envname options if not yet passed. Runs on *all* subcommands.
+    void _process_env() {
         for(const Option_p &opt : options_) {
             if(opt->count() == 0 && !opt->envname_.empty()) {
                 char *buffer = nullptr;
@@ -1338,18 +1334,52 @@ class App {
             }
         }
 
-        // Process callbacks
+        for(App_p &sub : subcommands_) {
+            sub->_process_env();
+        }
+    }
+
+    /// Process callbacks. Runs on *all* subcommands.
+    void _process_callbacks() {
         for(const Option_p &opt : options_) {
             if(opt->count() > 0 && !opt->get_callback_run()) {
                 opt->run_callback();
             }
         }
 
-        // Verify required options
+        for(App_p &sub : subcommands_) {
+            sub->_process_callbacks();
+        }
+    }
+
+    /// Run help flag processing if any are found.
+    ///
+    /// The flags allow recursive calls to remember if there was a help flag on a parent.
+    void _process_help_flags(bool trigger_help = false, bool trigger_all_help = false) const {
+        const Option *help_ptr = get_help_ptr();
+        const Option *help_all_ptr = get_help_all_ptr();
+
+        if(help_ptr != nullptr && help_ptr->count() > 0)
+            trigger_help = true;
+        if(help_all_ptr != nullptr && help_all_ptr->count() > 0)
+            trigger_all_help = true;
+
+        // If there were parsed subcommands, call those. First subcommand wins if there are multiple ones.
+        if(!parsed_subcommands_.empty()) {
+            for(const App *sub : parsed_subcommands_)
+                sub->_process_help_flags(trigger_help, trigger_all_help);
+
+            // Only the final subcommand should call for help. All help wins over help.
+        } else if(trigger_all_help) {
+            throw CallForAllHelp();
+        } else if(trigger_help) {
+            throw CallForHelp();
+        }
+    }
+
+    /// Verify required options and cross requirements. Subcommands too (only if selected).
+    void _process_requirements() {
         for(const Option_p &opt : options_) {
-            // Exit if a help flag was passed (requirements not required in that case)
-            if(_any_help_flag())
-                break;
 
             // Required or partially filled
             if(opt->get_required() || opt->count() != 0) {
@@ -1375,7 +1405,26 @@ class App {
         if(require_subcommand_min_ > selected_subcommands.size())
             throw RequiredError::Subcommand(require_subcommand_min_);
 
-        // Convert missing (pairs) to extras (string only)
+        // Max error cannot occur, the extra subcommand will parse as an ExtrasError or a remaining item.
+
+        for(App_p &sub : subcommands_) {
+            if(sub->count() > 0)
+                sub->_process_requirements();
+        }
+    }
+
+    /// Process callbacks and such.
+    void _process() {
+        _process_ini();
+        _process_env();
+        _process_callbacks();
+        _process_help_flags();
+        _process_requirements();
+    }
+
+    /// Throw an error if anything is left over and should not be.
+    /// Modifies the args to fill in the missing items before throwing.
+    void _process_extras(std::vector<std::string> &args) {
         if(!(allow_extras_ || prefix_command_)) {
             size_t num_left_over = remaining_size();
             if(num_left_over > 0) {
@@ -1384,24 +1433,30 @@ class App {
             }
         }
 
-        if(parent_ == nullptr) {
-            args = remaining(false);
+        for(App_p &sub : subcommands_) {
+            if(sub->count() > 0)
+                sub->_process_extras(args);
         }
     }
 
-    /// Return True if a help flag detected (checks all parents) (only run if help called before subcommand)
-    bool _any_help_flag() const {
-        bool result = false;
-        const Option *help_ptr = get_help_ptr();
-        const Option *help_all_ptr = get_help_all_ptr();
-        if(help_ptr != nullptr && help_ptr->count() > 0)
-            result = true;
-        if(help_all_ptr != nullptr && help_all_ptr->count() > 0)
-            result = true;
-        if(parent_ != nullptr)
-            return result || parent_->_any_help_flag();
-        else
-            return result;
+    /// Internal parse function
+    void _parse(std::vector<std::string> &args) {
+        parsed_++;
+        bool positional_only = false;
+
+        while(!args.empty()) {
+            _parse_single(args, positional_only);
+        }
+
+        if(parent_ == nullptr) {
+            _process();
+
+            // Throw error if any items are left over (depending on settings)
+            _process_extras(args);
+
+            // Convert missing (pairs) to extras (string only)
+            args = remaining(false);
+        }
     }
 
     /// Parse one config param, return false if not found in any subcommand, remove if it is

include/CLI/Option.hpp

@@ -215,9 +215,6 @@ class Option : public OptionBase<Option> {
     /// Options store a callback to do all the work
     callback_t callback_;
 
-    /// Options can short-circuit for help options or similar (called before parsing is validated)
-    bool short_circuit_{false};
-
     ///@}
     /// @name Parsing results
     ///@{
@@ -423,14 +420,6 @@ class Option : public OptionBase<Option> {
         return this;
     }
 
-    /// Options with a short circuit set will run this function before parsing is finished.
-    ///
-    /// This is set on help functions, for example, to escape the normal validation.
-    Option *short_circuit(bool value = true) {
-        short_circuit_ = value;
-        return this;
-    }
-
     ///@}
     /// @name Accessors
     ///@{
@@ -450,9 +439,6 @@ class Option : public OptionBase<Option> {
     /// The default value (for help printing)
     std::string get_defaultval() const { return defaultval_; }
 
-    /// See if this is supposed to short circuit (skip validation, INI, etc) (Used for help flags)
-    bool get_short_circuit() const { return short_circuit_; }
-
     /// Get the callback function
     callback_t get_callback() const { return callback_; }
 
@@ -556,6 +542,8 @@ class Option : public OptionBase<Option> {
     /// Process the callback
     void run_callback() {
 
+        callback_run_ = true;
+
         // Run the validators (can change the string)
         if(!validators_.empty()) {
             for(std::string &result : results_)

tests/AppTest.cpp

@@ -1520,3 +1520,25 @@ TEST_F(TApp, EmptyOptionFail) {
     args = {"--each", "that"};
     run();
 }
+
+TEST_F(TApp, BeforeRequirements) {
+    app.add_flag_function("-a", [](size_t) { throw CLI::Success(); });
+    app.add_flag_function("-b", [](size_t) { throw CLI::CallForHelp(); });
+
+    args = {"extra"};
+    EXPECT_THROW(run(), CLI::ExtrasError);
+
+    args = {"-a", "extra"};
+    EXPECT_THROW(run(), CLI::Success);
+
+    args = {"-b", "extra"};
+    EXPECT_THROW(run(), CLI::CallForHelp);
+
+    // These run in definition order.
+    args = {"-a", "-b", "extra"};
+    EXPECT_THROW(run(), CLI::Success);
+
+    // Currently, the original order is not preserved when calling callbacks
+    // args = {"-b", "-a", "extra"};
+    // EXPECT_THROW(run(), CLI::CallForHelp);
+}

tests/SubcommandTest.cpp

@@ -64,21 +64,19 @@ TEST_F(TApp, MultiSubFallthrough) {
     EXPECT_TRUE(app.got_subcommand(sub1));
     EXPECT_TRUE(*sub1);
     EXPECT_TRUE(sub1->parsed());
+    EXPECT_EQ(sub1->count(), (size_t)1);
 
     EXPECT_TRUE(app.got_subcommand("sub2"));
     EXPECT_TRUE(app.got_subcommand(sub2));
     EXPECT_TRUE(*sub2);
 
     app.require_subcommand();
-
     run();
 
     app.require_subcommand(2);
-
     run();
 
     app.require_subcommand(1);
-
     EXPECT_THROW(run(), CLI::ExtrasError);
 
     args = {"sub1"};
@@ -90,6 +88,7 @@ TEST_F(TApp, MultiSubFallthrough) {
     EXPECT_TRUE(*sub1);
     EXPECT_FALSE(*sub2);
     EXPECT_FALSE(sub2->parsed());
+    EXPECT_EQ(sub2->count(), (size_t)0);
 
     EXPECT_THROW(app.got_subcommand("sub3"), CLI::OptionNotFound);
 }
@@ -736,3 +735,45 @@ TEST_F(ManySubcommands, Unlimited) {
     run();
     EXPECT_EQ(app.remaining(true), vs_t());
 }
+
+TEST_F(ManySubcommands, HelpFlags) {
+
+    args = {"-h"};
+
+    EXPECT_THROW(run(), CLI::CallForHelp);
+
+    args = {"sub2", "-h"};
+
+    EXPECT_THROW(run(), CLI::CallForHelp);
+
+    args = {"-h", "sub2"};
+
+    EXPECT_THROW(run(), CLI::CallForHelp);
+}
+
+TEST_F(ManySubcommands, MaxCommands) {
+
+    app.require_subcommand(2);
+
+    args = {"sub1", "sub2"};
+    EXPECT_NO_THROW(run());
+
+    // The extra subcommand counts as an extra
+    args = {"sub1", "sub2", "sub3"};
+    EXPECT_NO_THROW(run());
+    EXPECT_EQ(sub2->remaining().size(), 1);
+
+    // Currently, setting sub2 to throw causes an extras error
+    // In the future, would passing on up to app's extras be better?
+
+    app.allow_extras(false);
+    sub1->allow_extras(false);
+    sub2->allow_extras(false);
+
+    args = {"sub1", "sub2"};
+
+    EXPECT_NO_THROW(run());
+
+    args = {"sub1", "sub2", "sub3"};
+    EXPECT_THROW(run(), CLI::ExtrasError);
+}

tests/app_helper.hpp

@@ -16,6 +16,7 @@ struct TApp : public ::testing::Test {
     input_t args;
 
     void run() {
+        // It is okay to re-parse - clear is called automatically before a parse.
         input_t newargs = args;
         std::reverse(std::begin(newargs), std::end(newargs));
         app.parse(newargs);