You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2026/03/02 12:38:04 UTC
[PR] ArgParser Fix: apply default values after dependency validation [trafficserver]
brbzull0 opened a new pull request, #12934:
URL: https://github.com/apache/trafficserver/pull/12934
ArgParser: apply default values after dependency validation
`validate_dependencies()` incorrectly triggers on options that have default values but were not explicitly passed by the user.
The root cause is that `append_option_data()` populates default values into the Arguments map before `validate_dependencies()` runs. When `validate_dependencies()` calls `ret.get(key)` for an option with a default, the lookup finds the entry and sets `_is_called = true`, making the option appear "used" even though the user never specified it on the command line.
Fix by extracting the default-value loop into `apply_option_defaults()` and calling it after `validate_dependencies()` in `parse()`.
Test also checks for `Case sensitive short options` which I needed to make sure they are working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ArgParser Fix: apply default values after dependency validation [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #12934:
URL: https://github.com/apache/trafficserver/pull/12934#discussion_r2877142395
##########
src/tscore/unit_tests/test_ArgParser.cc:
##########
@@ -148,3 +148,72 @@ TEST_CASE("Invoke test", "[invoke]")
parsed_data.invoke();
REQUIRE(global == 2);
}
+
+TEST_CASE("Case sensitive short options", "[parse]")
+{
+ ts::ArgParser cs_parser;
+ cs_parser.add_global_usage("test_prog [--SWITCH]");
+
+ // Add a command with two options that differ only in case: -t and -T
+ ts::ArgParser::Command &cmd = cs_parser.add_command("process", "process data");
+ cmd.add_option("--tag", "-t", "a label", "", 1, "");
+ cmd.add_option("--threshold", "-T", "a numeric value", "", 1, "100");
+
+ ts::Arguments parsed;
+
+ // Use lowercase -t: should set "tag" only
+ const char *argv1[] = {"test_prog", "process", "-t", "my_tag", nullptr};
+ parsed = cs_parser.parse(argv1);
+ REQUIRE(parsed.get("tag") == true);
+ REQUIRE(parsed.get("tag").value() == "my_tag");
+ // threshold should still have its default
+ REQUIRE(parsed.get("threshold").value() == "100");
+
+ // Use uppercase -T: should set "threshold" only
+ const char *argv2[] = {"test_prog", "process", "-T", "200", nullptr};
+ parsed = cs_parser.parse(argv2);
+ REQUIRE(parsed.get("threshold") == true);
+ REQUIRE(parsed.get("threshold").value() == "200");
+ // tag should be empty (no default)
+ REQUIRE(parsed.get("tag").value() == "");
+
+ // Use both -t and -T together
+ const char *argv3[] = {"test_prog", "process", "-t", "foo", "-T", "500", nullptr};
+ parsed = cs_parser.parse(argv3);
+ REQUIRE(parsed.get("tag") == true);
+ REQUIRE(parsed.get("tag").value() == "foo");
+ REQUIRE(parsed.get("threshold") == true);
+ REQUIRE(parsed.get("threshold").value() == "500");
+}
+
+TEST_CASE("with_required does not trigger on default values", "[parse]")
+{
+ ts::ArgParser parser;
+ parser.add_global_usage("test_prog [OPTIONS]");
+
+ ts::ArgParser::Command &cmd = parser.add_command("scan", "scan targets");
+ cmd.add_option("--tag", "-t", "a label", "", 1, "");
+ cmd.add_option("--verbose", "-v", "enable verbose output");
+ cmd.add_option("--threshold", "-T", "a numeric value", "", 1, "100").with_required("--verbose");
Review Comment:
no need. Common pattern. The shadowing is deliberate and correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ArgParser Fix: apply default values after dependency validation [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #12934:
URL: https://github.com/apache/trafficserver/pull/12934#discussion_r2877146801
##########
include/tscore/ArgParser.h:
##########
@@ -226,6 +226,8 @@ class ArgParser
void validate_mutex_groups(Arguments &ret) const;
// Helper method to validate option dependencies
void validate_dependencies(Arguments &ret) const;
+ // Helper method to apply default values for options not explicitly set
+ void apply_option_defaults(Arguments &ret);
Review Comment:
`const` makes sense. @copilot really???? const_cast?? 🤮
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ArgParser Fix: apply default values after dependency validation [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12934:
URL: https://github.com/apache/trafficserver/pull/12934#discussion_r2875045532
##########
include/tscore/ArgParser.h:
##########
@@ -226,6 +226,8 @@ class ArgParser
void validate_mutex_groups(Arguments &ret) const;
// Helper method to validate option dependencies
void validate_dependencies(Arguments &ret) const;
+ // Helper method to apply default values for options not explicitly set
+ void apply_option_defaults(Arguments &ret);
Review Comment:
`apply_option_defaults()` can be declared `const` since it doesn't modify the `Command` object; making it `const` aligns with `validate_mutex_groups()` / `validate_dependencies()` and prevents accidental state changes later.
```suggestion
void apply_option_defaults(Arguments &ret);
void apply_option_defaults(Arguments &ret) const
{
const_cast<Command *>(this)->apply_option_defaults(ret);
}
```
##########
src/tscore/unit_tests/test_ArgParser.cc:
##########
@@ -148,3 +148,72 @@ TEST_CASE("Invoke test", "[invoke]")
parsed_data.invoke();
REQUIRE(global == 2);
}
+
+TEST_CASE("Case sensitive short options", "[parse]")
+{
+ ts::ArgParser cs_parser;
+ cs_parser.add_global_usage("test_prog [--SWITCH]");
+
+ // Add a command with two options that differ only in case: -t and -T
+ ts::ArgParser::Command &cmd = cs_parser.add_command("process", "process data");
+ cmd.add_option("--tag", "-t", "a label", "", 1, "");
+ cmd.add_option("--threshold", "-T", "a numeric value", "", 1, "100");
+
+ ts::Arguments parsed;
+
+ // Use lowercase -t: should set "tag" only
+ const char *argv1[] = {"test_prog", "process", "-t", "my_tag", nullptr};
+ parsed = cs_parser.parse(argv1);
+ REQUIRE(parsed.get("tag") == true);
+ REQUIRE(parsed.get("tag").value() == "my_tag");
+ // threshold should still have its default
+ REQUIRE(parsed.get("threshold").value() == "100");
+
+ // Use uppercase -T: should set "threshold" only
+ const char *argv2[] = {"test_prog", "process", "-T", "200", nullptr};
+ parsed = cs_parser.parse(argv2);
+ REQUIRE(parsed.get("threshold") == true);
+ REQUIRE(parsed.get("threshold").value() == "200");
+ // tag should be empty (no default)
+ REQUIRE(parsed.get("tag").value() == "");
+
+ // Use both -t and -T together
+ const char *argv3[] = {"test_prog", "process", "-t", "foo", "-T", "500", nullptr};
+ parsed = cs_parser.parse(argv3);
+ REQUIRE(parsed.get("tag") == true);
+ REQUIRE(parsed.get("tag").value() == "foo");
+ REQUIRE(parsed.get("threshold") == true);
+ REQUIRE(parsed.get("threshold").value() == "500");
+}
+
+TEST_CASE("with_required does not trigger on default values", "[parse]")
+{
+ ts::ArgParser parser;
+ parser.add_global_usage("test_prog [OPTIONS]");
+
+ ts::ArgParser::Command &cmd = parser.add_command("scan", "scan targets");
+ cmd.add_option("--tag", "-t", "a label", "", 1, "");
+ cmd.add_option("--verbose", "-v", "enable verbose output");
+ cmd.add_option("--threshold", "-T", "a numeric value", "", 1, "100").with_required("--verbose");
Review Comment:
This test introduces a local variable named `parser` which shadows the file-scope `ts::ArgParser parser` used by other test cases in this file. Renaming the local variable (e.g., `local_parser`) would avoid confusion and reduce the risk of accidental misuse during future edits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ArgParser Fix: apply default values after dependency validation [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12934:
URL: https://github.com/apache/trafficserver/pull/12934#discussion_r2872275648
##########
include/tscore/ArgParser.h:
##########
@@ -226,6 +226,8 @@ class ArgParser
void validate_mutex_groups(Arguments &ret) const;
// Helper method to validate option dependencies
void validate_dependencies(Arguments &ret) const;
+ // Helper method to apply default values for options not explicitly set
+ void apply_option_defaults(Arguments &ret);
Review Comment:
`apply_option_defaults()` doesn't modify `Command` state (it only reads `_option_list` and writes to the `Arguments` output). Marking it `const` would better reflect its behavior and keep it consistent with the other validation helpers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ArgParser Fix: apply default values after dependency validation [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 merged PR #12934:
URL: https://github.com/apache/trafficserver/pull/12934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org