feat(option): add skip_excluded to consume hidden options silently - #182
feat(option): add skip_excluded to consume hidden options silently#182Seele-Official wants to merge 2 commits into
Conversation
When an option is excluded by visibility/include/exclude flags and skip_excluded is enabled, the parser consumes the option exactly as if it were accepted (including joined/separate/remaining values) but never yields a ParsedArg and never reports a missing-value error. Excluded matches still lose to visible duplicates with the same spelling, and grouped short options expand through hidden flags without output. Wire the option unit tests into unit_tests (they were gated on an undeclared 'option' config and the .cpp file was not matched by the deco glob) and add coverage for the new behavior.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesExcluded option parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant OptTable
participant OptionIterator
Caller->>OptTable: parse with skip_excluded enabled
OptTable->>OptTable: scan visible and excluded matches
OptTable->>OptionIterator: consume excluded option and value
OptionIterator-->>Caller: yield visible arguments only
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/deco/option/table.cc (1)
409-435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winContinue scanning past an excluded match to find a visible duplicate.
is_known_optionreturns as soon as it finds the first matching entry inrange.scan_and_acceptwas extended in this PR to keep scanning past an excluded match so a visible option with the same spelling can still win (see the "Keep scanning" comment at Lines 327-338).is_known_optionwas not given the same treatment.If a hidden option's spelling collides with a later visible option's spelling in
option_infos, andskip_excludedisfalse, this function returnsfalsefor that spelling even though a visible duplicate exists further in the range and would actually be matched byscan_and_accept.consume_unknown_values(used withgreedy_unknown) then incorrectly swallows that argument as a value of a preceding unknown/greedy option instead of treating it as a known-option boundary.🐛 Proposed fix to keep scanning past excluded entries
OptionRef opt(*s, table); - if(options.excludes(opt)) - return options.skip_excluded; - return true; + if(options.excludes(opt)) { + if(options.skip_excluded) + return true; + continue; + } + return true; } return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/deco/option/table.cc` around lines 409 - 435, Update is_known_option to continue scanning after a matching option that options.excludes(opt) reports as excluded, rather than immediately returning options.skip_excluded. Return true when a later non-excluded duplicate is found, while preserving the existing matching-kind and argument-length checks and returning the current skip_excluded result only when no visible match remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/deco/option_tests.cpp`:
- Around line 760-771: Sort the option entries used by the tablegen path in
kSkipOptInfos by the OptNameLess ordering before scan_and_accept applies
std::lower_bound, or provide a name-sorted array for that path. Preserve the
existing duplicate-visible option selection and ensure searches such as -d begin
at the correct range position.
---
Outside diff comments:
In `@src/deco/option/table.cc`:
- Around line 409-435: Update is_known_option to continue scanning after a
matching option that options.excludes(opt) reports as excluded, rather than
immediately returning options.skip_excluded. Return true when a later
non-excluded duplicate is found, while preserving the existing matching-kind and
argument-length checks and returning the current skip_excluded result only when
no visible match remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8a207e8-b509-4bb1-9864-1275309c6418
📒 Files selected for processing (4)
include/kota/deco/option/table.hsrc/deco/option/table.cctests/unit/deco/option_tests.cppxmake.lua
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7e7eb0499
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… boundary Two review findings on skip_excluded: 1. In grouped-short-option mode a later excluded duplicate could overwrite an earlier visible fallback (last-wins), so parsing -ab with a visible -a followed by a hidden -a dropped the visible -a. The fallback recording now keeps a visible candidate over an excluded duplicate, mirroring the non-grouped duplicate preference. 2. is_known_option returned immediately for the first excluded match, which changed the default (skip_excluded off) greedy-unknown behavior: with a hidden -x preceding a visible -x, --unknown -x swallowed -x as a value instead of treating it as a boundary. Excluded entries are skipped again unless skip_excluded is enabled, in which case they remain recognized boundaries. Adds regression tests for both cases plus the all-excluded greedy case.
1 participant
When an option is excluded by visibility/include/exclude flags and skip_excluded is enabled, the parser consumes the option exactly as if it were accepted (including joined/separate/remaining values) but never yields a ParsedArg and never reports a missing-value error. Excluded matches still lose to visible duplicates with the same spelling, and grouped short options expand through hidden flags without output.
Wire the option unit tests into unit_tests (they were gated on an undeclared 'option' config and the .cpp file was not matched by the deco glob) and add coverage for the new behavior.
Summary by CodeRabbit
New Features
Bug Fixes