From 954c93d5856de034ed9c40eef1cc6bc63686226e Mon Sep 17 00:00:00 2001
From: Henry Fredrick Schreiner <henry.fredrick.schreiner@cern.ch>
Date: Tue, 28 Nov 2017 13:27:41 -0500
Subject: [PATCH] Using more powerful policy system, to allow git-like -m
 options

---
 include/CLI/App.hpp    |  5 +++--
 include/CLI/Option.hpp | 47 +++++++++++++++++++++---------------------
 tests/AppTest.cpp      | 28 +++++++++++++++++++++++--
 tests/CreationTest.cpp | 14 ++++++++-----
 4 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/include/CLI/App.hpp b/include/CLI/App.hpp
index 861c901b..90b0d18f 100644
--- a/include/CLI/App.hpp
+++ b/include/CLI/App.hpp
@@ -394,7 +394,8 @@ class App {
         return opt;
     }
 
-    /// Bool version - defaults to allowing multiple passings, but can be forced to one if `take_last(false)` is used.
+    /// Bool version - defaults to allowing multiple passings, but can be forced to one if
+    /// `multi_option_policy(CLI::MultiOptionPolicy::Throw)` is used.
     template <typename T, enable_if_t<is_bool<T>::value, detail::enabler> = detail::dummy>
     Option *add_flag(std::string name,
                      T &count, ///< A variable holding true if passed
@@ -410,7 +411,7 @@ class App {
         if(opt->get_positional())
             throw IncorrectConstruction("Flags cannot be positional");
         opt->set_custom_option("", 0);
-        opt->take_last();
+        opt->multi_option_policy(CLI::MultiOptionPolicy::TakeLast);
         return opt;
     }
 
diff --git a/include/CLI/Option.hpp b/include/CLI/Option.hpp
index 990440f6..76210cd1 100644
--- a/include/CLI/Option.hpp
+++ b/include/CLI/Option.hpp
@@ -26,6 +26,8 @@ class App;
 
 using Option_p = std::unique_ptr<Option>;
 
+enum class MultiOptionPolicy { Throw, TakeLast, TakeFirst, Join };
+
 template <typename CRTP> class OptionBase {
     friend App;
 
@@ -39,14 +41,14 @@ template <typename CRTP> class OptionBase {
     /// Ignore the case when matching (option, not value)
     bool ignore_case_{false};
 
-    /// Only take the last argument (requires `expected_ == 1`)
-    bool last_{false};
+    /// Policy for multiple arguments when `expected_ == 1`  (can be set on bool flags, too)
+    MultiOptionPolicy multi_option_policy_{MultiOptionPolicy::Throw};
 
     template <typename T> void copy_to(T *other) const {
         other->group(group_);
         other->required(required_);
         other->ignore_case(ignore_case_);
-        other->take_last(last_);
+        other->multi_option_policy(multi_option_policy_);
     }
 
   public:
@@ -79,8 +81,8 @@ template <typename CRTP> class OptionBase {
     /// The status of ignore case
     bool get_ignore_case() const { return ignore_case_; }
 
-    /// The status of the take last flag
-    bool get_take_last() const { return last_; }
+    /// The status of the multi option policy
+    MultiOptionPolicy get_multi_option_policy() const { return multi_option_policy_; }
 };
 
 class OptionDefaults : public OptionBase<OptionDefaults> {
@@ -90,8 +92,8 @@ class OptionDefaults : public OptionBase<OptionDefaults> {
     // Methods here need a different implementation if they are Option vs. OptionDefault
 
     /// Take the last argument if given multiple times
-    OptionDefaults *take_last(bool value = true) {
-        last_ = value;
+    OptionDefaults *multi_option_policy(MultiOptionPolicy value) {
+        multi_option_policy_ = value;
         return this;
     }
 
@@ -213,8 +215,9 @@ class Option : public OptionBase<Option> {
             throw IncorrectConstruction("Cannot set 0 expected, use a flag instead");
         else if(!changeable_)
             throw IncorrectConstruction("You can only change the expected arguments for vectors");
-        else if(last_)
-            throw IncorrectConstruction("You can't change expected arguments after you've set take_last!");
+        else if(value != 1 && multi_option_policy_ != MultiOptionPolicy::Throw)
+            throw IncorrectConstruction(
+                "You can't change expected arguments after you've changed the multi option policy!");
 
         expected_ = value;
         return this;
@@ -304,10 +307,10 @@ class Option : public OptionBase<Option> {
     }
 
     /// Take the last argument if given multiple times
-    Option *take_last(bool value = true) {
+    Option *multi_option_policy(MultiOptionPolicy value) {
         if(get_expected() != 0 && get_expected() != 1)
-            throw IncorrectConstruction("take_last only works for flags and single value options!");
-        last_ = value;
+            throw IncorrectConstruction("multi_option_policy only works for flags and single value options!");
+        multi_option_policy_ = value;
         return this;
     }
 
@@ -315,15 +318,9 @@ class Option : public OptionBase<Option> {
     /// @name Accessors
     ///@{
 
-    /// True if this is a required option
-    bool get_required() const { return required_; }
-
     /// The number of arguments the option expects
     int get_expected() const { return expected_; }
 
-    /// The status of the take last flag
-    bool get_take_last() const { return last_; }
-
     /// True if this has a default value
     int get_default() const { return default_; }
 
@@ -336,9 +333,6 @@ class Option : public OptionBase<Option> {
     /// True if option has description
     bool has_description() const { return description_.length() > 0; }
 
-    /// Get the group of this option
-    const std::string &get_group() const { return group_; }
-
     /// Get the description
     const std::string &get_description() const { return description_; }
 
@@ -443,10 +437,17 @@ class Option : public OptionBase<Option> {
         }
 
         bool local_result;
-        // If take_last, only operate on the final item
-        if(last_) {
+
+        // Operation depends on the policy setting
+        if(multi_option_policy_ == MultiOptionPolicy::TakeLast) {
             results_t partial_result = {results_.back()};
             local_result = !callback_(partial_result);
+        } else if(multi_option_policy_ == MultiOptionPolicy::TakeFirst) {
+            results_t partial_result = {results_.at(0)};
+            local_result = !callback_(partial_result);
+        } else if(multi_option_policy_ == MultiOptionPolicy::Join) {
+            results_t partial_result = {detail::join(results_, "\n")};
+            local_result = !callback_(partial_result);
         } else {
             if((expected_ > 0 && results_.size() != static_cast<size_t>(expected_)) ||
                (expected_ < 0 && results_.size() < static_cast<size_t>(-expected_)))
diff --git a/tests/AppTest.cpp b/tests/AppTest.cpp
index d171c6ed..6caa966e 100644
--- a/tests/AppTest.cpp
+++ b/tests/AppTest.cpp
@@ -169,7 +169,7 @@ TEST_F(TApp, BoolAndIntFlags) {
 
 TEST_F(TApp, BoolOnlyFlag) {
     bool bflag;
-    app.add_flag("-b", bflag)->take_last(false);
+    app.add_flag("-b", bflag)->multi_option_policy(CLI::MultiOptionPolicy::Throw);
 
     args = {"-b"};
     EXPECT_NO_THROW(run());
@@ -221,7 +221,7 @@ TEST_F(TApp, DefaultOpts) {
 TEST_F(TApp, TakeLastOpt) {
 
     std::string str;
-    app.add_option("--str", str)->take_last();
+    app.add_option("--str", str)->multi_option_policy(CLI::MultiOptionPolicy::TakeLast);
 
     args = {"--str=one", "--str=two"};
 
@@ -230,6 +230,30 @@ TEST_F(TApp, TakeLastOpt) {
     EXPECT_EQ(str, "two");
 }
 
+TEST_F(TApp, TakeFirstOpt) {
+
+    std::string str;
+    app.add_option("--str", str)->multi_option_policy(CLI::MultiOptionPolicy::TakeFirst);
+
+    args = {"--str=one", "--str=two"};
+
+    run();
+
+    EXPECT_EQ(str, "one");
+}
+
+TEST_F(TApp, JoinOpt) {
+
+    std::string str;
+    app.add_option("--str", str)->multi_option_policy(CLI::MultiOptionPolicy::Join);
+
+    args = {"--str=one", "--str=two"};
+
+    run();
+
+    EXPECT_EQ(str, "one\ntwo");
+}
+
 TEST_F(TApp, MissingValueNonRequiredOpt) {
     int count;
     app.add_option("-c,--count", count);
diff --git a/tests/CreationTest.cpp b/tests/CreationTest.cpp
index 0eaea225..0c708711 100644
--- a/tests/CreationTest.cpp
+++ b/tests/CreationTest.cpp
@@ -152,14 +152,14 @@ TEST_F(TApp, IncorrectConstructionVectorAsFlag) {
 TEST_F(TApp, IncorrectConstructionVectorTakeLast) {
     std::vector<int> vec;
     auto cat = app.add_option("--vec", vec);
-    EXPECT_THROW(cat->take_last(), CLI::IncorrectConstruction);
+    EXPECT_THROW(cat->multi_option_policy(CLI::MultiOptionPolicy::TakeLast), CLI::IncorrectConstruction);
 }
 
 TEST_F(TApp, IncorrectConstructionTakeLastExpected) {
     std::vector<int> vec;
     auto cat = app.add_option("--vec", vec);
     cat->expected(1);
-    ASSERT_NO_THROW(cat->take_last());
+    ASSERT_NO_THROW(cat->multi_option_policy(CLI::MultiOptionPolicy::TakeLast));
     EXPECT_THROW(cat->expected(2), CLI::IncorrectConstruction);
 }
 
@@ -301,16 +301,20 @@ TEST_F(TApp, OptionFromDefaults) {
 TEST_F(TApp, OptionFromDefaultsSubcommands) {
     // Initial defaults
     EXPECT_FALSE(app.option_defaults()->get_required());
-    EXPECT_FALSE(app.option_defaults()->get_take_last());
+    EXPECT_EQ(app.option_defaults()->get_multi_option_policy(), CLI::MultiOptionPolicy::Throw);
     EXPECT_FALSE(app.option_defaults()->get_ignore_case());
     EXPECT_EQ(app.option_defaults()->get_group(), "Options");
 
-    app.option_defaults()->required()->take_last()->ignore_case()->group("Something");
+    app.option_defaults()
+        ->required()
+        ->multi_option_policy(CLI::MultiOptionPolicy::TakeLast)
+        ->ignore_case()
+        ->group("Something");
 
     auto app2 = app.add_subcommand("app2");
 
     EXPECT_TRUE(app2->option_defaults()->get_required());
-    EXPECT_TRUE(app2->option_defaults()->get_take_last());
+    EXPECT_EQ(app2->option_defaults()->get_multi_option_policy(), CLI::MultiOptionPolicy::TakeLast);
     EXPECT_TRUE(app2->option_defaults()->get_ignore_case());
     EXPECT_EQ(app2->option_defaults()->get_group(), "Something");
 }