Skip to content

Commit

Permalink
conflicting option names (#1049)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
phlptp and pre-commit-ci[bot] authored Jun 3, 2024
1 parent 08d840b commit 4ecbdd8
Show file tree
Hide file tree
Showing 7 changed files with 111 additions and 11 deletions.
4 changes: 2 additions & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
24 changes: 21 additions & 3 deletions include/CLI/impl/App_inl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Expand Down Expand Up @@ -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;
}
}
}

Expand Down
9 changes: 5 additions & 4 deletions include/CLI/impl/Option_inl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
}
Expand Down
14 changes: 13 additions & 1 deletion tests/AppTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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();
}

Expand Down
68 changes: 68 additions & 0 deletions tests/ConfigFileTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"};
Expand Down Expand Up @@ -2818,6 +2879,13 @@ TEST_CASE_METHOD(TApp, "IniDisableFlagOverride", "[config]") {
CHECK(tmpini3.c_str() == app.get_config_ptr()->as<std::string>());
}

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};
Expand Down
2 changes: 1 addition & 1 deletion tests/FuzzFailTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions tests/fuzzFail/fuzz_app_file_fail40
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
config ' '

0 comments on commit 4ecbdd8

Please sign in to comment.