fix(parse): keep implicit default after a default-only flag - #1418
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe parser now commits to the default subcommand after a default-only flag, even when later words match sibling commands. Variadic flag values stop at higher-precedence subcommands without discarding the route. Tests, fixtures, documentation, and spec exports cover the updated behavior. ChangesDefault subcommand routing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The updated default-command routing is covered across implementations and remains mergeable. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The routing changes, tests, corpus fixtures, and documentation are within issue
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let parsed = Em::parse_from(&["-u", "query"].map(OsStr::new)).unwrap(); | ||
| let Some(Commands::Install(install)) = parsed.command else { | ||
| panic!("expected install: a default-only flag already committed the line") | ||
| }; | ||
| assert!(install.update); | ||
| assert_eq!(install.package.as_deref(), Some("query")); |
There was a problem hiding this comment.
The updated test accepts -u query as install with package query, but corpus/16-default-subcommand-flags.json still expects unknown_flag for that input in default-flags-sibling-wins. The default-flags-alias-wins and default-flags-explicit-prefix vectors likewise retain the old error expectations for -u q and -u install. Both corpus conformance suites compare parser results against these expectations, so the intended routing change breaks existing assertions. Update these vectors and their descriptions to assert the new default-command bindings.
Knowledge Base Used: Cross-implementation conformance
A sibling name before any default-only flag still stays on the parent (`em query`, `em -p query`). After one, later words are the default command's args, so `em -u pkg` is not UnknownFlag -u when `pkg` is also a command. The same lookahead is in argv, usage-lib, and go/argv. Binding corpus vectors that encoded the old selector rule now expect a parse. Closes jdx#1417.
de569d0 to
d213686
Compare
2 participants
Once a default-only flag has been seen, later words are the default command's args.
em query/em -p querystill select the sibling.em -u queryisem install -u queryeven whenqueryis also a command, so a default command whose positionals are an open name set (pkg,use,search) does not getUnknownFlag -u.Closes #1417.
Summary by CodeRabbit
Bug Fixes
Documentation
New Features