fix: improve some confusing error situations with config files (#781)

* add some more tests for coverage and fix some confusing error situations with config files.

* style: pre-commit.ci fixes

* fix warning

* ci: fix coverage

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <[email protected]>

Author: 3 people

.github/workflows/tests.yml

@@ -20,6 +20,8 @@ jobs:
         precompile: ["ON", "OFF"]
     steps:
       - uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
 
       - name: Get LCov
         run: |
@@ -50,12 +52,10 @@ jobs:
           lcov --list coverage.info
         working-directory: build
 
-      - name: Upload coverage
-        run: |
-          curl -Os https://uploader.codecov.io/latest/linux/codecov
-          chmod +x codecov
-          ./codecov
-        working-directory: build
+      - uses: codecov/codecov-action@v3
+        with:
+          fail_ci_if_error: true
+          working-directory: build
 
   clang-tidy:
     name: Clang-Tidy

include/CLI/impl/App_inl.hpp

@@ -1384,16 +1384,23 @@ CLI11_INLINE bool App::_parse_single_config(const ConfigItem &item, std::size_t
     if(op->empty()) {
 
         if(op->get_expected_min() == 0) {
-            // Flag parsing
-            auto res = config_formatter_->to_flag(item);
-            res = op->get_flag_value(item.name, res);
+            if(item.inputs.size() <= 1) {
+                // Flag parsing
+                auto res = config_formatter_->to_flag(item);
+                res = op->get_flag_value(item.name, res);
 
-            op->add_result(res);
-
-        } else {
-            op->add_result(item.inputs);
-            op->run_callback();
+                op->add_result(res);
+                return true;
+            }
+            if(static_cast<int>(item.inputs.size()) > op->get_items_expected_max()) {
+                if(op->get_items_expected_max() > 1) {
+                    throw ArgumentMismatch::AtMost(item.fullname(), op->get_items_expected_max(), item.inputs.size());
+                }
+                throw ConversionError::TooManyInputsFlag(item.fullname());
+            }
         }
+        op->add_result(item.inputs);
+        op->run_callback();
     }
 
     return true;

tests/AppTest.cpp

@@ -981,7 +981,9 @@ TEST_CASE_METHOD(TApp, "emptyVectorReturn", "[app]") {
 
     std::vector<std::string> strs;
     std::vector<std::string> strs2;
+    std::vector<std::string> strs3;
     auto *opt1 = app.add_option("--str", strs)->required()->expected(0, 2);
+    app.add_option("--str3", strs3)->expected(1, 3);
     app.add_option("--str2", strs2);
     args = {"--str"};
 
@@ -1004,6 +1006,11 @@ TEST_CASE_METHOD(TApp, "emptyVectorReturn", "[app]") {
 
     CHECK_NOTHROW(run());
     CHECK(strs.empty());
+    opt1->required(false);
+    args = {"--str3", "{}"};
+
+    CHECK_NOTHROW(run());
+    CHECK_FALSE(strs3.empty());
 }
 
 TEST_CASE_METHOD(TApp, "RequiredOptsDoubleShort", "[app]") {

tests/ConfigFileTest.cpp

@@ -1017,17 +1017,19 @@ TEST_CASE_METHOD(TApp, "TOMLStringVector", "[config]") {
         out << "zero1=[]\n";
         out << "zero2={}\n";
         out << "zero3={}\n";
+        out << "zero4=[\"{}\",\"\"]\n";
         out << "nzero={}\n";
         out << "one=[\"1\"]\n";
         out << "two=[\"2\",\"3\"]\n";
         out << "three=[\"1\",\"2\",\"3\"]\n";
     }
 
-    std::vector<std::string> nzero, zero1, zero2, zero3, one, two, three;
+    std::vector<std::string> nzero, zero1, zero2, zero3, zero4, one, two, three;
     app.add_option("--zero1", zero1)->required()->expected(0, 99)->default_str("{}");
     app.add_option("--zero2", zero2)->required()->expected(0, 99)->default_val(std::vector<std::string>{});
     // if no default is specified the argument results in an empty string
     app.add_option("--zero3", zero3)->required()->expected(0, 99);
+    app.add_option("--zero4", zero4)->required()->expected(0, 99);
     app.add_option("--nzero", nzero)->required();
     app.add_option("--one", one)->required();
     app.add_option("--two", two)->required();
@@ -1038,6 +1040,7 @@ TEST_CASE_METHOD(TApp, "TOMLStringVector", "[config]") {
     CHECK(zero1 == std::vector<std::string>({}));
     CHECK(zero2 == std::vector<std::string>({}));
     CHECK(zero3 == std::vector<std::string>({""}));
+    CHECK(zero4 == std::vector<std::string>({"{}"}));
     CHECK(nzero == std::vector<std::string>({"{}"}));
     CHECK(one == std::vector<std::string>({"1"}));
     CHECK(two == std::vector<std::string>({"2", "3"}));
@@ -1735,6 +1738,23 @@ TEST_CASE_METHOD(TApp, "IniFlagDual", "[config]") {
     CHECK_THROWS_AS(run(), CLI::ConversionError);
 }
 
+TEST_CASE_METHOD(TApp, "IniVectorMax", "[config]") {
+
+    TempFile tmpini{"TestIniTmp.ini"};
+
+    std::vector<std::string> v1;
+    app.config_formatter(std::make_shared<CLI::ConfigINI>());
+    app.add_option("--vec", v1)->expected(0, 2);
+    app.set_config("--config", tmpini);
+
+    {
+        std::ofstream out{tmpini};
+        out << "vec=[a,b,c]" << std::endl;
+    }
+
+    CHECK_THROWS_AS(run(), CLI::ArgumentMismatch);
+}
+
 TEST_CASE_METHOD(TApp, "IniShort", "[config]") {
 
     TempFile tmpini{"TestIniTmp.ini"};

tests/HelpTest.cpp

@@ -974,6 +974,16 @@ TEST_CASE("THelp: GroupOrder", "[help]") {
     CHECK(aee_loc > zee_loc);
 }
 
+TEST_CASE("THelp: GroupNameError", "[help]") {
+    CLI::App app;
+
+    auto *f1 = app.add_flag("--one");
+    auto *f2 = app.add_flag("--two");
+
+    CHECK_THROWS_AS(f1->group("evil group name\non two lines"), CLI::IncorrectConstruction);
+    CHECK_THROWS_AS(f2->group(std::string(5, '\0')), CLI::IncorrectConstruction);
+}
+
 TEST_CASE("THelp: ValidatorsText", "[help]") {
     CLI::App app;
 

tests/SetTest.cpp

@@ -492,6 +492,23 @@ TEST_CASE_METHOD(TApp, "FailSet", "[set]") {
     CHECK_THROWS_AS(run(), CLI::ValidationError);
 }
 
+TEST_CASE_METHOD(TApp, "shortStringCheck", "[set]") {
+
+    std::string choice;
+    app.add_option("-q,--quick", choice)->check([](const std::string &v) {
+        if(v.size() > 5) {
+            return std::string{"string too long"};
+        }
+        return std::string{};
+    });
+
+    args = {"--quick", "3"};
+    CHECK_NOTHROW(run());
+
+    args = {"--quick=hello_goodbye"};
+    CHECK_THROWS_AS(run(), CLI::ValidationError);
+}
+
 TEST_CASE_METHOD(TApp, "FailMutableSet", "[set]") {
 
     int choice{0};