From 4ecbdd83e51eb49d1e4e99bea7c55e36c0426d34 Mon Sep 17 00:00:00 2001 From: Philip Top Date: Mon, 3 Jun 2024 09:23:07 -0700 Subject: [PATCH] conflicting option names (#1049) Take the configurability of an option into account when determining ambiguous names and conflicts. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .pre-commit-config.yaml | 4 +- include/CLI/impl/App_inl.hpp | 24 ++++++++-- include/CLI/impl/Option_inl.hpp | 9 ++-- tests/AppTest.cpp | 14 +++++- tests/ConfigFileTest.cpp | 68 +++++++++++++++++++++++++++++ tests/FuzzFailTest.cpp | 2 +- tests/fuzzFail/fuzz_app_file_fail40 | 1 + 7 files changed, 111 insertions(+), 11 deletions(-) create mode 100644 tests/fuzzFail/fuzz_app_file_fail40 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 10a885f94..25527be63 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -87,7 +87,7 @@ repos: exclude: .pre-commit-config.yaml - repo: https://github.com/codespell-project/codespell - rev: v2.2.6 + rev: v2.3.0 hooks: - id: codespell - args: ["-L", "atleast,ans,doub,inout"] + args: ["-L", "atleast,ans,doub,inout,AtMost"] diff --git a/include/CLI/impl/App_inl.hpp b/include/CLI/impl/App_inl.hpp index a06955c49..b8d241b34 100644 --- a/include/CLI/impl/App_inl.hpp +++ b/include/CLI/impl/App_inl.hpp @@ -174,19 +174,19 @@ CLI11_INLINE Option *App::add_option(std::string option_name, } auto *op = get_option_no_throw(test_name); - if(op != nullptr) { + if(op != nullptr && op->get_configurable()) { throw(OptionAlreadyAdded("added option positional name matches existing option: " + test_name)); } } else if(parent_ != nullptr) { for(auto &ln : myopt.lnames_) { auto *op = parent_->get_option_no_throw(ln); - if(op != nullptr) { + if(op != nullptr && op->get_configurable()) { throw(OptionAlreadyAdded("added option matches existing positional option: " + ln)); } } for(auto &sn : myopt.snames_) { auto *op = parent_->get_option_no_throw(sn); - if(op != nullptr) { + if(op != nullptr && op->get_configurable()) { throw(OptionAlreadyAdded("added option matches existing positional option: " + sn)); } } @@ -1490,6 +1490,24 @@ CLI11_INLINE bool App::_parse_single_config(const ConfigItem &item, std::size_t } if(op == nullptr) { op = get_option_no_throw(item.name); + } else if(!op->get_configurable()) { + auto *testop = get_option_no_throw(item.name); + if(testop != nullptr && testop->get_configurable()) { + op = testop; + } + } + } else if(!op->get_configurable()) { + if(item.name.size() == 1) { + auto *testop = get_option_no_throw("-" + item.name); + if(testop != nullptr && testop->get_configurable()) { + op = testop; + } + } + if(!op->get_configurable()) { + auto *testop = get_option_no_throw(item.name); + if(testop != nullptr && testop->get_configurable()) { + op = testop; + } } } diff --git a/include/CLI/impl/Option_inl.hpp b/include/CLI/impl/Option_inl.hpp index 1b51000b0..72f3dfad1 100644 --- a/include/CLI/impl/Option_inl.hpp +++ b/include/CLI/impl/Option_inl.hpp @@ -311,26 +311,27 @@ CLI11_INLINE void Option::run_callback() { CLI11_NODISCARD CLI11_INLINE const std::string &Option::matching_name(const Option &other) const { static const std::string estring; + bool bothConfigurable = configurable_ && other.configurable_; for(const std::string &sname : snames_) { if(other.check_sname(sname)) return sname; - if(other.check_lname(sname)) + if(bothConfigurable && other.check_lname(sname)) return sname; } for(const std::string &lname : lnames_) { if(other.check_lname(lname)) return lname; - if(lname.size() == 1) { + if(lname.size() == 1 && bothConfigurable) { if(other.check_sname(lname)) { return lname; } } } - if(snames_.empty() && lnames_.empty() && !pname_.empty()) { + if(bothConfigurable && snames_.empty() && lnames_.empty() && !pname_.empty()) { if(other.check_sname(pname_) || other.check_lname(pname_) || pname_ == other.pname_) return pname_; } - if(other.snames_.empty() && other.fnames_.empty() && !other.pname_.empty()) { + if(bothConfigurable && other.snames_.empty() && other.fnames_.empty() && !other.pname_.empty()) { if(check_sname(other.pname_) || check_lname(other.pname_) || (pname_ == other.pname_)) return other.pname_; } diff --git a/tests/AppTest.cpp b/tests/AppTest.cpp index fd72ecd7b..94a56de29 100644 --- a/tests/AppTest.cpp +++ b/tests/AppTest.cpp @@ -555,6 +555,18 @@ TEST_CASE_METHOD(TApp, "NumberFlags", "[app]") { CHECK(7 == val); } +TEST_CASE_METHOD(TApp, "doubleDashH", "[app]") { + + int val{0}; + // test you can add a --h option and it doesn't conflict with the help + CHECK_NOTHROW(app.add_flag("--h", val)); + + auto *topt = app.add_flag("-t"); + CHECK_THROWS_AS(app.add_flag("--t"), CLI::OptionAlreadyAdded); + topt->configurable(false); + CHECK_NOTHROW(app.add_flag("--t")); +} + TEST_CASE_METHOD(TApp, "DisableFlagOverrideTest", "[app]") { int val{0}; @@ -2180,7 +2192,7 @@ TEST_CASE_METHOD(TApp, "NeedsTrue", "[app]") { args = {"--string", "val_with_opt1", "--opt1"}; run(); - args = {"--opt1", "--string", "val_with_opt1"}; // order is not revelant + args = {"--opt1", "--string", "val_with_opt1"}; // order is not relevant run(); } diff --git a/tests/ConfigFileTest.cpp b/tests/ConfigFileTest.cpp index 2102c6cf2..c239e2eaf 100644 --- a/tests/ConfigFileTest.cpp +++ b/tests/ConfigFileTest.cpp @@ -1109,6 +1109,67 @@ TEST_CASE_METHOD(TApp, "IniOverwrite", "[config]") { CHECK(two == 99); } +TEST_CASE_METHOD(TApp, "hInConfig", "[config]") { + + TempFile tmpini{"TestIniTmp.ini"}; + { + std::ofstream out{tmpini}; + out << "[default]" << '\n'; + out << "h=99" << '\n'; + } + + std::string next = "TestIniTmp.ini"; + app.set_config("--conf", next); + int two{7}; + app.add_option("--h", two); + + run(); + + CHECK(two == 99); +} + +TEST_CASE_METHOD(TApp, "notConfigurableOptionOverload", "[config]") { + + TempFile tmpini{"TestIniTmp.ini"}; + { + std::ofstream out{tmpini}; + out << "[default]" << '\n'; + out << "m=99" << '\n'; + } + + std::string next = "TestIniTmp.ini"; + app.set_config("--conf", next); + int two{7}; + int three{5}; + app.add_option("--m", three)->configurable(false); + app.add_option("-m", two); + + run(); + CHECK(three == 5); + CHECK(two == 99); +} + +TEST_CASE_METHOD(TApp, "notConfigurableOptionOverload2", "[config]") { + + TempFile tmpini{"TestIniTmp.ini"}; + { + std::ofstream out{tmpini}; + out << "[default]" << '\n'; + out << "m=99" << '\n'; + } + + std::string next = "TestIniTmp.ini"; + app.set_config("--conf", next); + int two{7}; + int three{5}; + app.add_option("-m", three)->configurable(false); + app.add_option("m", two); + + run(); + CHECK(three == 5); + CHECK(two == 99); +} + TEST_CASE_METHOD(TApp, "IniRequired", "[config]") { TempFile tmpini{"TestIniTmp.ini"}; @@ -2818,6 +2879,13 @@ TEST_CASE_METHOD(TApp, "IniDisableFlagOverride", "[config]") { CHECK(tmpini3.c_str() == app.get_config_ptr()->as()); } +TEST_CASE("fclear", "[config]") { + // mainly to clear up some warnings + (void)fclear1; + (void)fclear2; + (void)fclear3; +} + TEST_CASE_METHOD(TApp, "TomlOutputSimple", "[config]") { int v{0}; diff --git a/tests/FuzzFailTest.cpp b/tests/FuzzFailTest.cpp index 124c8f428..532a643c6 100644 --- a/tests/FuzzFailTest.cpp +++ b/tests/FuzzFailTest.cpp @@ -66,7 +66,7 @@ TEST_CASE("app_file_gen_fail") { CLI::FuzzApp fuzzdata; auto app = fuzzdata.generateApp(); - int index = GENERATE(range(1, 40)); + int index = GENERATE(range(1, 41)); std::string optionString, flagString; auto parseData = loadFailureFile("fuzz_app_file_fail", index); if(parseData.size() > 25) { diff --git a/tests/fuzzFail/fuzz_app_file_fail40 b/tests/fuzzFail/fuzz_app_file_fail40 new file mode 100644 index 000000000..789c0a791 --- /dev/null +++ b/tests/fuzzFail/fuzz_app_file_fail40 @@ -0,0 +1 @@ + config ' ' \ No newline at end of file