-
Notifications
You must be signed in to change notification settings - Fork 27
fix(lib): add missing child node support to arg parser #489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @jdx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of argument definitions by extending the parser to recognize a wider range of properties as child nodes. This change resolves previous limitations where only a few properties were supported, ensuring that Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for additional child nodes (help, long_help, help_long, help_md, required, var, var_min, var_max, hide, double_dash) inside {} blocks for arg definitions in the argument parser. Previously, only choices, env, and default were supported as child nodes.
Changes:
- Extended
SpecArg::parse()to handle 10 additional child node types - Added comprehensive test coverage for the new child node support
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "long_help" => arg.help_long = Some(child.arg(0)?.ensure_string()?), | ||
| "help_long" => arg.help_long = Some(child.arg(0)?.ensure_string()?), |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both long_help and help_long set the same field arg.help_long. This duplication suggests these are aliases, but it creates ambiguity about which one is the canonical name. Consider documenting this aliasing behavior or consolidating to a single name to improve clarity.
The arg parser only supported choices, env, and default as child nodes
inside {} blocks. Add support for help, long_help, help_long, help_md,
required, var, var_min, var_max, hide, and double_dash, matching the
pattern already used in the flag parser.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
366cb88 to
bcb2fa9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds support for several previously unsupported child nodes in arg definitions, improving consistency within the parser. The changes are accompanied by new tests. I have two suggestions: one to refactor a small piece of duplicated code using an or-pattern, and another to enhance the new tests to cover both aliases for long_help, ensuring better test coverage.
| "long_help" => arg.help_long = Some(child.arg(0)?.ensure_string()?), | ||
| "help_long" => arg.help_long = Some(child.arg(0)?.ensure_string()?), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve maintainability and reduce code duplication, you can use an or-pattern to handle both long_help and help_long in a single match arm.
| "long_help" => arg.help_long = Some(child.arg(0)?.ensure_string()?), | |
| "help_long" => arg.help_long = Some(child.arg(0)?.ensure_string()?), | |
| "long_help" | "help_long" => arg.help_long = Some(child.arg(0)?.ensure_string()?), |
| assert!(svc_arg.var); | ||
| assert_eq!(svc_arg.var_min, Some(0)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_arg_long_help_child_node() { | ||
| let spec = Spec::parse( | ||
| &Default::default(), | ||
| r#" | ||
| arg "<input>" { | ||
| help "Input file" | ||
| long_help "Extended help text for input" | ||
| } | ||
| "#, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve test coverage, this test should verify that both long_help and its alias help_long are parsed correctly as child nodes. You can do this by adding another argument definition to the spec string and asserting its properties.
fn test_arg_long_help_child_node() {
let spec = Spec::parse(
&Default::default(),
r#"
arg "<input>" {
help "Input file"
long_help "Extended help text for input"
}
arg "<input2>" {
help "Input file 2"
help_long "Extended help text for input 2"
}
"#,
)
.unwrap();
let input_arg = spec.cmd.args.iter().find(|a| a.name == "input").unwrap();
assert_eq!(input_arg.help, Some("Input file".to_string()));
assert_eq!(input_arg.help_long, Some("Extended help text for input".to_string()));
let input_arg2 = spec.cmd.args.iter().find(|a| a.name == "input2").unwrap();
assert_eq!(input_arg2.help, Some("Input file 2".to_string()));
assert_eq!(input_arg2.help_long, Some("Extended help text for input 2".to_string()));
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
==========================================
- Coverage 70.71% 69.58% -1.13%
==========================================
Files 47 47
Lines 6716 6789 +73
Branches 6716 6789 +73
==========================================
- Hits 4749 4724 -25
- Misses 1277 1299 +22
- Partials 690 766 +76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
### 🐛 Bug Fixes - **(lib)** add missing child node support to arg parser by [@jdx](https://github.com/jdx) in [#489](#489) - **(release)** write release notes to file instead of capturing stdout by [@jdx](https://github.com/jdx) in [#488](#488) ### 📚 Documentation - add opengraph meta tags by [@jdx](https://github.com/jdx) in [#486](#486) ### 🔍 Other Changes - add tone calibration to release notes prompt by [@jdx](https://github.com/jdx) in [#483](#483) ### 📦️ Dependency Updates - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#487](#487)
Summary
help,long_help,help_long,help_md,required,var,var_min,var_max,hide, anddouble_dashas child nodes inside{}blocks forargdefinitionschoices,env, anddefaultwere supported as child nodes, causing parse errors for other propertiesSpecFlag::parse()Test plan
test_arg_child_nodestest verifyinghelp,choices,var, andvar_minas child nodestest_arg_long_help_child_nodetest verifyinghelpandlong_helpas child nodes🤖 Generated with Claude Code
Note
Low Risk
Additive parsing support in
SpecArg::parseplus tests; low risk aside from potentially changing behavior for specs that previously errored or relied on unsupported child nodes.Overview
SpecArg::parsenow accepts common argument attributes (e.g.help,long_help/help_long,help_md,required,var+var_min/var_max,hide,double_dash) as child nodes insidearg { ... }blocks, instead of erroring on them.Adds new tests verifying mixed child-node parsing (including
choices) andlong_helpparsing for args.Written by Cursor Bugbot for commit bcb2fa9. This will update automatically on new commits. Configure here.