feat: Support for Option Group Name #71
Conversation
Features: * all Non-Blank (based on full whitespace) Group Names are shown as-is in Usage * all Blank Group Names get a count-based default Group Name within Usage * all Groupless Options are shown before Grouped Options * relative order of all Options within a Group is preserved * relative order of all Groups is preserved
Objective: * All Group Names are properly padded with newlines in the presence of Groupless Options * All Group Names are properly padded with newlines in the absence of Groupless Options * Blank Group Names are not rendered as-is in the presence of Groupless Options * Blank Group Names are not rendered as-is in the absence of Groupless Options * Non-Blank Group Names are rendered as-is in the presence of Groupless Options * Non-Blank Group Names are rendered as-is in the absence of Groupless Options * Blank Group Names get a count-based default Group Name in the presence of Groupless Options * Blank Group Names get a count-based default Group Name in the absence of Groupless Options * All Groupless Options are shown before Grouped Options * Relative order of all Options within a Group is preserved * Relative order of all Groups is preserved
📝 WalkthroughWalkthroughAdds support for grouping options when adding them to an ArgParser (groupless options emitted first, groups preserved with separators and fallback names for blank headers), enables grouping by default for prepareOptionsForParsing, preserves prior runtime behavior for single-option insertion, and adds tests validating usage text rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as BetterCommandRunner
participant Options as addOptionsToParser
participant Arg as ArgParser
participant Usage as UsageFormatter
Runner->>Options: addOptionsToParser(opts, argParser, addGroupSeparators: true)
Note right of Options: Phase 1 — Partition\n• groupless (preserve order)\n• ordered groups (preserve order)
rect rgba(220,245,255,0.35)
Note right of Options: Phase 2 — Emit groupless (original order)
loop each groupless option
Options->>Arg: addOption(option)
end
end
rect rgba(235,245,220,0.35)
Note right of Options: Phase 3 — Emit groups (original group order)
loop each group (visible only)
Options->>Arg: addSeparator(header) /* header uses fallback if blank */
loop each option in group
Options->>Arg: addOption(option)
end
end
end
Runner->>Arg: request usage()
Arg-->>Usage: formatted usage text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (4)
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
🧹 Nitpick comments (2)
packages/config/lib/src/config/options.dart (1)
822-831: Consider extracting helper functions for better testability.The inline helper functions work well, but extracting them to module-level or a separate utility would improve testability and reusability. Additionally, consider using more idiomatic Dart naming (
numberOfOptionGroupsinstead ofnOptionGroups).If refactoring, you could extract to:
bool _isNotBlank(String name) => name.trim().isNotEmpty; String _buildFallbackGroupName(int groupCounter, int totalGroups) => 'Option Group${totalGroups > 1 ? " $groupCounter" : ""}';packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart (1)
8-21: Consider more idiomatic Dart naming.The variable
nBlankNameExamplesuses Hungarian notation (nprefix for count), which is not idiomatic Dart. Consider renaming toblankNameExampleCountor simply computing it inline where needed.- final nBlankNameExamples = blankNameExamples.length; + final blankNameExampleCount = blankNameExamples.length;Then update usages:
- for (var i = 0; i < nBlankNameExamples; ++i) + for (var i = 0; i < blankNameExampleCount; ++i)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart(1 hunks)packages/config/lib/src/config/options.dart(1 hunks)
🔇 Additional comments (7)
packages/config/lib/src/config/options.dart (2)
807-821: LGTM! Clean separation of groupless and grouped options.The logic correctly categorizes options into groupless and grouped, maintaining insertion order. This sets up the two-phase processing approach (groupless first, then grouped with headers) as intended.
836-862: Group ordering is already tested
The existing test "Relative order of all Groups is preserved" inoption_group_usage_text_test.dartverifies that groups appear in declaration order.packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart (5)
1-36: LGTM! Well-structured test setup.The helper functions (
buildSeparatorView,buildMockArgName, etc.) make the tests highly readable and maintainable. The constants for blank name examples are comprehensive.
37-86: Tests validate the newline hack behavior.These tests verify that group names are properly padded with newlines. However, they depend on the workaround implemented in
addOptionsToParser(lines 842-853 ofoptions.dart). When the upstream args package bug is fixed and the hack is removed, these tests will need to be updated or may fail.This is by design per the implementation comments, but ensure there's a clear link between the hack removal and test updates.
Consider adding a comment in these test groups referencing the hack:
group('All Group Names are properly padded with newlines', () { // NOTE: These tests validate behavior that includes a workaround // for an args package padding bug. See options.dart lines 842-853. test(/* ... */); });
190-255: Excellent coverage of fallback naming behavior.These tests thoroughly validate the count-based default group names for blank groups, including the interaction with both groupless and grouped options. The use of
stringContainsInOrdereffectively verifies the ordering.
257-293: Critical test validating groupless-before-grouped ordering.This test confirms the core requirement that groupless options appear before grouped ones. The assertion correctly uses
stringContainsInOrderto verify the sequence.
295-357: Strong validation of relative ordering preservation.Both tests (within-group and between-group) effectively validate that the implementation maintains the declaration order of options and groups. The use of counters to track expected ordering is clever and ensures correctness.
indraneel12
changed the title
indraneel12
changed the title
To match the existing Code Style.
AND-condition Optimization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/lib/src/config/options.dart (1)
813-817: Avoid repeated list copying when appending to group entries.Each call to
[...value, opt]clones the entire list, introducing avoidable allocations. Mutating the existing list in place keeps the order while being allocation-free.if (group != null) { optionGroups.update( group, - (final value) => [...value, opt], + (final value) => value..add(opt), ifAbsent: () => [opt], ); } else {
As suggested (nitpick) by the Review Bot on my Pull Request.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/lib/src/config/options.dart (1)
807-822: LGTM! Clean categorization of options into groups.The two-phase collection approach (groupless options first, then grouped options) is well-structured and maintains proper ordering. The use of an ordered map for
optionGroupsand an ordered list forgrouplessOptionscorrectly preserves insertion order.Optional minor refactor: Line 822 could use
optionGroups.lengthinstead ofoptionGroups.keys.lengthfor brevity, as they're equivalent:- final nOptionGroups = optionGroups.keys.length; + final nOptionGroups = optionGroups.length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/config/lib/src/config/options.dart(1 hunks)
🔇 Additional comments (3)
packages/config/lib/src/config/options.dart (3)
824-830: LGTM! Well-designed helper functions.The local helper functions enhance code readability and maintainability. The
buildFallbackGroupNamefunction is particularly well-thought-out—it adds a numeric suffix only when there are multiple groups, providing clear fallback names like "Option Group" or "Option Group 1", "Option Group 2", etc.
832-833: LGTM! Groupless options processed first.This correctly ensures that options without a group appear before any grouped options in the usage text, which is the intended behavior.
835-862: LGTM! Proper group handling with fallback names.The implementation correctly:
- Processes groups in their declared order
- Provides sensible fallback names for blank group names using
buildFallbackGroupName- Uses
addSeparatorto ensure proper visual separation between groups- Maintains the relative order of options within each group
The use of
isNotBlank(givenGroupName)at line 857 elegantly handles the case where a group name is blank or whitespace-only, falling back to the generated name. This addresses the PR objective of supporting option group names robustly.
Anyways, I reckon it's a good improvement lol
A flag to choose between the previous behavior of `addOptionsToParser` (default to not affect the existing Codebase) and the new implementation that handles Group Option Name logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/lib/src/config/options.dart (1)
832-838: Optional: Consider extractingdefaultFallbackGroupNameto file scope.The
defaultFallbackGroupNameconstant is currently defined inside the function (line 833), which means it gets re-declared on every invocation. While Dart's compiler may optimize this, extracting it as a private top-level constant would make the optimization explicit:const _defaultFallbackGroupName = 'Option Group';Then reference it in the function:
String buildFallbackGroupName(final int counter) => '$_defaultFallbackGroupName${optionGroups.length > 1 ? " $counter" : ""}';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/config/lib/src/config/config_parser.dart(1 hunks)packages/config/lib/src/config/options.dart(2 hunks)
🔇 Additional comments (5)
packages/config/lib/src/config/config_parser.dart (1)
213-213: LGTM: Correct use ofaddGroupSeparators: falsefor incremental option addition.When options are added one at a time through
_addOption, passingaddGroupSeparators: falsecorrectly prevents separator insertion between individual options. This preserves the intended single-pass behavior for dynamic option registration while allowing grouped separators only during the final two-phase parsing flow.packages/config/lib/src/config/options.dart (4)
725-725: LGTM: Two-phase parsing enabled for option preparation.The call to
addOptionsToParserwithaddGroupSeparators: truecorrectly activates the two-phase grouping flow during option preparation. This ensures group separators are inserted at the appropriate points in the usage text while maintaining backward compatibility for other call sites.
803-814: LGTM: Clean design for backward-compatible grouping.The optional
addGroupSeparatorsparameter with afalsedefault preserves existing behavior for callers that don't specify it, while the early-return path (lines 808-814) efficiently handles the non-grouped case without executing the grouping logic. This design cleanly separates the single-pass and two-phase flows.
816-841: LGTM: Well-structured grouping preparation.The grouping logic correctly:
- Uses ordered collections to preserve declaration order (lines 817-818)
- Categorizes options by group membership (lines 819-830)
- Defines clear helper functions for consistent processing (lines 832-838)
- Adds groupless options first to ensure they appear before grouped options in usage text (lines 840-841)
The
buildFallbackGroupNamehelper (lines 837-838) thoughtfully omits the counter suffix when there's only one group, improving UX.
843-870: LGTM: Proper group separator insertion and blank name handling.The grouped option processing correctly:
- Iterates through groups in declaration order (line 845)
- Handles blank group names by falling back to generated names (lines 847-848, 864-866)
- Inserts separators before each group's options (lines 863-866)
- Adds all options within each group in order (lines 868-869)
The fallback logic at line 865 using
isNotBlankensures users always see meaningful group headers even when group names are empty or whitespace-only.Note: The newline workaround (lines 850-861) was already discussed in previous review threads and is understood to be intentional technical debt tracked for removal when the upstream args package bug is resolved.
Addresses the Review Bot's nitpick comment on `defaultFallbackGroupName` in a better way.
I have raised [an Issue](dart-lang/core#917) for `package:args` and updated this Codebase with the expected behavior and adjusted (+refactored) the Test Cases accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/lib/src/config/options.dart (1)
803-858: Consider adding function documentation.While the implementation is solid, consider adding documentation to
addOptionsToParserto clarify:
- The purpose and behavior of
addGroupSeparators- When and why to use
defaultFallbackGroupName- The ordering guarantees (groupless first, then groups in insertion order)
This would help future maintainers understand the function's contract without reading the implementation.
Example:
/// Adds options to the argument parser. /// /// If [addGroupSeparators] is true, options are organized into groups: /// - Groupless options are added first in their original order /// - Grouped options follow, with each group preceded by a separator /// - Groups appear in order of first encounter /// - Blank group names use [defaultFallbackGroupName] with optional numbering /// /// If [addGroupSeparators] is false, all options are added in their original order /// without any grouping or separators. void addOptionsToParser( final Iterable<OptionDefinition> argNameOpts, final ArgParser argParser, { final bool addGroupSeparators = false, final String defaultFallbackGroupName = 'Option Group', }) { // ... implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart(1 hunks)packages/config/lib/src/config/options.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart
🔇 Additional comments (4)
packages/config/lib/src/config/options.dart (4)
725-725: LGTM! Group separators enabled in standard path.Enabling
addGroupSeparators: truein the standard preparation path is the correct choice for user-facing commands. The implementation maintains backward compatibility through the default parameter value inaddOptionsToParser.
803-808: LGTM! Function signature is backward compatible.The addition of optional named parameters with sensible defaults maintains backward compatibility while enabling the new grouping feature. The parameter names are clear and the defaults are appropriate.
809-815: LGTM! Clean backward compatibility path.The early return when
addGroupSeparatorsis false provides a clean separation between the original behavior and the new grouping logic, avoiding any performance overhead when grouping is not needed.
817-857: LGTM! Well-structured grouping implementation.The grouping logic is clear and handles edge cases appropriately:
- Partitioning phase (lines 817-831): Efficiently separates groupless and grouped options in a single pass
- Helper functions (lines 833-838): Improve readability and maintain consistency
- Execution flow (lines 840-857): Correctly processes groupless options first, then grouped options with separators
Edge cases handled correctly:
- Blank group names fall back to numbered names
- Single group gets "Option Group" without number
- Multiple groups get "Option Group 1", "Option Group 2", etc.
- Groups with identical names are merged (per
OptionGroup.operator==)- Map insertion order ensures groups appear in order of first encounter
- more rigorous (e.g. newline precision and order) - more readable (e.g. highly modular) - easily enhanceable (e.g. via almost direct copy-paste for similar and additional Test Cases)
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Good work! And good that you minded supporting backwards compatibility for the drop-in replacement wrapper, and logged an issue to the upstream package!
A couple of comments, and one major point to change re not filling in group names.
|
In addition, please enter a couple of output examples in the top PR description, so the reader can see what the outcome of this change brings. |
- entirely removed Fallback Group Name feature - updated corresponding Test Cases with the actual expected behavior
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/config/lib/src/config/options.dart (3)
805-814: Clean backward-compatible implementation.The optional parameter with a
falsedefault maintains existing behavior while enabling the new grouping functionality. The early return is a clean pattern that avoids nested conditionals.Consider removing the comment on line 808 as the conditional and return statement clearly convey the intent. As per previous feedback, self-evident comments can be omitted.
- // Plain Option-addition without any Group Logic if (!addGroupSeparators) {
817-818: Address previous feedback: verify map ordering and remove self-evident comment.Two unresolved items from previous review:
Map ordering (line 817): While Dart's default map literal
{}produces aLinkedHashMapthat maintains insertion order (since Dart 2.0+), this behavior should be explicitly verified or documented. Consider adding a brief note explaining that insertion order is guaranteed by Dart's map implementation.Self-evident comment (line 818): The comment
// ordered listis self-evident since Dart lists are always ordered. Per previous feedback, this can be removed.- final optionGroups = <OptionGroup, List<OptionDefinition>>{}; // ordered map + // Dart map literals maintain insertion order (LinkedHashMap by default) + final optionGroups = <OptionGroup, List<OptionDefinition>>{}; - final grouplessOptions = <OptionDefinition>[]; // ordered list + final grouplessOptions = <OptionDefinition>[];
832-843: Logic is correct; consider consolidating comments.The implementation correctly adds groupless options first, then processes each group with its separator and options in order. Line 841 appropriately uses
group.namedirectly without fallback, aligning with previous feedback that group names are mandatory.The comments on lines 832, 836, and 839 somewhat restate what the code clearly expresses. Per previous feedback about omitting self-evident comments, consider consolidating into a single higher-level comment or removing them:
- // Helpers for consistent processing and validation void addOne(final OptionDefinition x) => x.option._addToArgParser(argParser); void addAll(final List<OptionDefinition> options) => options.forEach(addOne); - // Add all Groupless Options first (in order) addAll(grouplessOptions); - // Add all Groups (in order) and their Options (in order) + // Process groups: add separator followed by grouped options optionGroups.forEach((final group, final options) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart(1 hunks)packages/config/lib/src/config/options.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart
🔇 Additional comments (1)
packages/config/lib/src/config/options.dart (1)
725-725: LGTM! Grouped option rendering enabled.The change to pass
addGroupSeparators: truecorrectly enables the new grouped option functionality in the main flow, aligning with the PR objectives.
Explicit mention of Ordered Containers.
Ensures that the relative order of all Groups and Options is preserved.
Reflects the correct Option Group Name behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/config/lib/src/config/options.dart (1)
803-846: LGTM! Well-structured implementation with good backwards compatibility.The refactoring clearly separates the old behavior (simple iteration) from the new grouping logic. The use of ordered containers preserves option and group order as required, and the approach of adding groupless options first followed by grouped options with separators aligns with the PR objectives.
Minor nitpick: The comment on Line 834 mentions "validation" but the helper functions only perform processing (adding options to the parser). Actual validation occurs in
validateOptions.Consider this diff for accuracy:
- // Helpers for consistent processing and validation + // Helpers for consistent processing void addOne(final OptionDefinition x) => x.option._addToArgParser(argParser); void addAll(final List<OptionDefinition> options) => options.forEach(addOne);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/config/lib/src/config/options.dart(3 hunks)
🔇 Additional comments (2)
packages/config/lib/src/config/options.dart (2)
44-45: LGTM!The documentation update clearly explains the purpose of the group name in usage output and provides helpful formatting guidance.
725-725: Grouping change is backward-compatible for config usage
Grouping separators are only added when options have a non-null group (config options default to null), so the usage output and existing tests/documentation remain unchanged.
Also addresses a nitpick comment by the Review Bot.
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Good stuff! Just one more minor comment.
- inlined small helper functions - comments emphasize the intent behind modified implemention - added external reference for explanation of Ordered Map in Dart
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/config/lib/src/config/options.dart (1)
816-821: Trim self-evident comment and external links.This reiterates Dart’s ordered Map/List semantics; prior feedback requested omitting such comments.
- // The following containers are ordered by default i.e. - // preserves insertion-order: - // - Map : https://stackoverflow.com/q/79786585/10251345 - // - List : https://api.dart.dev/dart-core/List-class.html final optionGroups = <OptionGroup, List<OptionDefinition>>{}; final grouplessOptions = <OptionDefinition>[];
🧹 Nitpick comments (2)
packages/config/lib/src/config/options.dart (2)
803-808: Add a brief doc comment for the new flag.Document that addGroupSeparators only affects usage output (adds ArgParser separators) and does not alter parsing. Minor API polish.
-void addOptionsToParser( +/// Adds named options to [argParser]. If [addGroupSeparators] is true, +/// inserts separators using each option's [OptionGroup.name] before the +/// group's options in the generated usage text. Parsing behavior is unchanged. +void addOptionsToParser( final Iterable<OptionDefinition> argNameOpts, final ArgParser argParser, { final bool addGroupSeparators = false, })
842-848: Avoid empty headers for groups with only hidden options.If a group’s options are all hidden, the header appears with no visible entries. Keep adding the options (so they parse), but skip the separator for such groups.
- optionGroups.forEach((final group, final groupedOptions) { - argParser.addSeparator(group.name); - for (final o in groupedOptions) { - o.option._addToArgParser(argParser); - } - }); + optionGroups.forEach((final group, final groupedOptions) { + final hasVisible = groupedOptions.any((o) => !o.option.hide); + if (hasVisible) { + argParser.addSeparator(group.name); + } + for (final o in groupedOptions) { + o.option._addToArgParser(argParser); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/config/lib/src/config/options.dart(3 hunks)
🔇 Additional comments (2)
packages/config/lib/src/config/options.dart (2)
44-47: Doc clarification looks good.Clear that name is used as the usage header; no issues.
719-726: Confirm default grouping in prepareOptionsForParsing callers.
prepareOptionsForParsing now passes addGroupSeparators=true by default. It’s invoked in:
- config/lib/src/config/options.dart (prepareForParsing)
- cli_tools/lib/src/better_command_runner/better_command_runner.dart:177
- cli_tools/lib/src/better_command_runner/better_command.dart:67
Ensure grouped usage headers are desired in all these contexts.
- for Groups with only Hidden Options - minor Comment improvements
- Test Cases for the recent fix (bug caught by the Review Bot) - minor Comment improvement
Combination of: - Groupless Options - Grouped Options - Hidden Groups
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart (7)
83-129: Also assert each visible group header appears exactly onceYou verify presence/absence, but not duplication. Add an occurrence check per visible group to catch regressions where headers could repeat.
Apply within each test by asserting a single match for a couple of groups:
- expect( - buildRunner(grouplessOptions + groupedOptions + hiddenGroups).usage, - expectation, - ); + final usage = + buildRunner(grouplessOptions + groupedOptions + hiddenGroups).usage; + expect(usage, expectation); + // Ensure visible group headers are not duplicated. + expect(RegExp(RegExp.escape(buildMockGroupName(1))) + .allMatches(usage) + .length, equals(1)); + expect(RegExp(RegExp.escape(buildMockGroupName(5))) + .allMatches(usage) + .length, equals(1));Repeat similarly in the “absence of Groupless Options” test if desired.
131-161: Relax newline padding assertion to avoid brittle failures (CRLF/format tweaks)Matching exactly “two newlines before + one after” is brittle. Prefer “one or more before + one after.”
- final expectation = allOf([ - for (var i = 5; i < 10; ++i) - contains(buildSeparatorView(buildMockGroupName(i))), - ]); + final expectation = allOf([ + for (var i = 5; i < 10; ++i) + // Allow one or more newlines before the header; require one after. + contains(RegExp(r'\n+' + RegExp.escape(buildMockGroupName(i)) + r'\n')), + ]);
163-191: Make “groupless before grouped” assertion line-ending agnostic and less formatting-sensitiveRelying on explicit newline tokens can be fragile across platforms or minor formatting changes. Compare indices instead.
- final expectation = stringContainsInOrder([ - '\n', - for (var i = 5; i < 10; ++i) ...[ - buildMockArgName(i), - '\n', - ], - '\n', - for (var i = 0; i < 5; ++i) ...[ - buildMockArgName(i), - '\n', - ], - '\n', - ]); - expect( - buildRunner(groupedOptions + grouplessOptions).usage, - expectation, - ); + final usage = buildRunner(groupedOptions + grouplessOptions) + .usage + .replaceAll('\r\n', '\n'); + final lastGrouplessIdx = + usage.lastIndexOf(buildMockArgName(9)); // last of 5..9 + final firstGroupedIdx = + usage.indexOf(buildMockArgName(0)); // first of 0..4 + expect(firstGroupedIdx, greaterThan(lastGrouplessIdx));
193-229: Reuse the same OptionGroup instances to avoid relying on name-based grouping semanticsCurrently each option calls buildMockGroup(i), creating new objects; if grouping ever used identity, this test wouldn’t validate intra-group ordering. Reuse group instances per i.
final groupedOptions = <OptionDefinition>[ - for (var i = 0; i < 3; ++i) + // Reuse the same OptionGroup instance per group. + for (var i = 0; i < 3; ++i) + for (var j = 0; j < 0; ++j) {} // no-op to preserve diff context + ]; + final groups = [buildMockGroup(0), buildMockGroup(1), buildMockGroup(2)]; + groupedOptions.addAll([ for (var j = 0; j < 5; ++j) buildMockOption( buildMockArgName(++testOptionCount), - buildMockGroup(i), + groups[0], ), - ]; + for (var j = 0; j < 5; ++j) + buildMockOption( + buildMockArgName(++testOptionCount), + groups[1], + ), + for (var j = 0; j < 5; ++j) + buildMockOption( + buildMockArgName(++testOptionCount), + groups[2], + ), + ]);Note: Any equivalent approach that reuses group instances per i works.
231-258: LGTM; order of group headers is validatedConfirms visible group headers appear in declared order. Consider also asserting no duplicate headers, similar to earlier comment, but optional.
260-349: Strengthen combined test by asserting single header occurrence for visible groupsYou already assert absence for fully hidden groups. Also ensure headers like “Group 1/2/5” appear exactly once.
- expect( - buildRunner(const <OptionDefinition>[ ... ]).usage, - allOf([ + final usage = buildRunner(const <OptionDefinition>[ ... ]).usage; + expect( + usage, + allOf([ stringContainsInOrder([ 'option-1', 'option-5', 'Group 1', 'option-2', 'option-4', 'Group 2', 'option-3', 'option-6', 'Group 5', 'option-11', ]), isNot(contains('Group 3')), isNot(contains('option-7')), isNot(contains('Group 4')), isNot(contains('option-8')), isNot(contains('option-9')), isNot(contains('option-10')), ]), ); + // Ensure visible group headers appear only once. + expect(RegExp(r'\bGroup 1\b').allMatches(usage).length, equals(1)); + expect(RegExp(r'\bGroup 2\b').allMatches(usage).length, equals(1)); + expect(RegExp(r'\bGroup 5\b').allMatches(usage).length, equals(1));
1-349: Add tests for anonymous/blank group names and whitespace-only namesPR objective mentions default naming for anonymous groups; this suite doesn’t cover it. Add scenarios for:
- Multiple OptionGroup('') groups: headers get distinct fallback names in order.
- OptionGroup(' '): trimmed treated as anonymous; same fallback semantics.
Would you like me to open a follow-up PR with tests? Minimal example to append at end of file:
+ test( + 'Anonymous/blank group names get fallback headers in order', + () { + final options = <OptionDefinition>[ + buildMockOption('groupless-a'), + // Two anonymous groups with visible options. + FlagOption(argName: 'anon-1a', group: const OptionGroup(''), helpText: 'h'), + FlagOption(argName: 'anon-1b', group: const OptionGroup(''), helpText: 'h'), + FlagOption(argName: 'anon-2a', group: const OptionGroup(''), helpText: 'h'), + // Whitespace-only should be treated as anonymous too. + FlagOption(argName: 'anon-ws-a', group: const OptionGroup(' '), helpText: 'h'), + ]; + final usage = buildRunner(options).usage.replaceAll('\r\n', '\n'); + // Expect at least two non-empty group headers after groupless options. + final headerMatches = RegExp(r'\n+\s*(.+\S)\s*\n').allMatches(usage).toList(); + expect(headerMatches.length, greaterThanOrEqualTo(2)); + // The two first fallback headers should be distinct and ordered. + final first = headerMatches[0].group(1)!; + final second = headerMatches[1].group(1)!; + expect(first, isNotEmpty); + expect(second, isNotEmpty); + expect(first, isNot(equals(second))); + // Ensure anonymous options appear under/after their respective headers (order check). + expect(usage.indexOf('anon-1a'), lessThan(usage.indexOf('anon-2a'))); + }, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart(1 hunks)
🔇 Additional comments (1)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart (1)
52-81: Good coverage for visible group headersValidates that named, visible groups render in usage both with and without groupless options. Looks solid.
- more compact Helper Functions - better Call Site readability
|
@coderabbitai I've been sadly noticing that you have a bug (visible through your comments and edits to PR Summary since yesterday). Please update your Context Window to recognize the fact that Fallback Group Name feature is no longer part of this Pull Request, as required above by a Package Admin. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 25231 @indraneel12, you're absolutely right, and I apologize for the confusion. I've reviewed the actual implementation in The actual implementation is straightforward:
I was incorrectly referencing the workaround described in issue #70, which mentioned assigning numbered default names, but that approach was not implemented in this PR. Thank you for catching this error and giving me the opportunity to correct my understanding. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart (4)
119-149: Avoid brittle assertions on exact newline paddingTests require exactly two preceding newlines for headers. Minor formatting tweaks could break them without changing semantics. Prefer a regex that allows “2 or more” newlines.
Example rewrite for expectation:
final expectation = allOf([ for (var i = 5; i < 10; ++i) matches(RegExp(r'\n{2,}${RegExp.escape(buildMockGroupName(i))}\n')), ]);
151-179: Make ordering assertion less fragile (remove newline tokens)The order test currently depends on explicit newline placement. Assert semantic order: all groupless args come before the first group header.
Apply this diff:
- final expectation = stringContainsInOrder([ - '\n', - for (var i = 5; i < 10; ++i) ...[ - buildMockArgName(i), - '\n', - ], - '\n', - for (var i = 0; i < 5; ++i) ...[ - buildMockArgName(i), - '\n', - ], - '\n', - ]); - expect( - buildRunner(groupedOptions + grouplessOptions).usage, - expectation, - ); + final usage = buildRunner(groupedOptions + grouplessOptions).usage; + expect( + usage, + stringContainsInOrder([ + for (var i = 5; i < 10; ++i) buildMockArgName(i), + // First visible group header should appear after all groupless options. + buildMockGroupName(0), + ]), + );
219-246: Add a test for non-contiguous options of the same group (header appears once, options consolidate)You verify group-order preservation, but here each option gets a new group. Add a case where the same group appears in disjoint positions to ensure a single header consolidates its options in order.
Example:
test('Group appears once and consolidates non-contiguous options', () { final options = <OptionDefinition>[ buildMockOption('a', 'G'), buildMockOption('b', null), buildMockOption('c', 'G'), ]; final usage = buildRunner(options).usage; expect(usage, contains('G')); // header present // Header appears exactly once. expect(RegExp(r'^\s*G\s*$', multiLine: true).allMatches(usage).length, equals(1)); expect(usage, stringContainsInOrder(['a', 'c'])); });
1-3: Missing test for anonymous/blank group names (fallback naming)PR notes numbered defaults for anonymous groups. Add a test where OptionGroup('') or whitespace name is used multiple times to assert headers render with fallback names and preserve group/option order.
I can draft this test if you confirm the expected fallback label format (e.g., “Group 1”, “Group 2”, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart(1 hunks)
🔇 Additional comments (4)
packages/cli_tools/test/better_command_runner/option_group_usage_text_test.dart (4)
20-37: Good test scaffolding and reuseHelper factories and runner builder keep tests clear and DRY. LGTM.
70-117: Hidden-group behavior is well coveredSolid checks: only groups with at least one visible option render; fully hidden groups and their options don’t. LGTM.
181-217: Option order within groups is well-validatedNice check that preserves intra-group option order while mixing with groupless options. LGTM.
248-287: Comprehensive scenario testCovers groupless, grouped, and hidden interactions with correct visibility and ordering. LGTM.
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
One final spelling comment.
Adjusted according to the guidelines of a Package Admin.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/config/lib/src/config/options.dart (2)
803-809: Document the addGroupSeparators switchAdd a brief doc comment describing semantics and default to help future callers.
-void addOptionsToParser( +/// Adds options to [argParser]. When [addGroupSeparators] is true, options are +/// grouped by [OptionGroup] and a separator with the group name is inserted +/// before each group that has at least one visible option. Groupless options +/// are added first in their original order. +void addOptionsToParser( final Iterable<OptionDefinition> argNameOpts, final ArgParser argParser, { final bool addGroupSeparators = false, })
816-821: Make ordering intent explicit; drop explanatory commentUse LinkedHashMap to encode insertion-order intent directly and remove the inline explanatory comment.
- // the following containers are ordered by default i.e. - // preserves insertion-order: - // - Map : https://stackoverflow.com/q/79786585/10251345 - // - List : https://api.dart.dev/dart-core/List-class.html - final optionGroups = <OptionGroup, List<OptionDefinition>>{}; - final grouplessOptions = <OptionDefinition>[]; + final optionGroups = LinkedHashMap<OptionGroup, List<OptionDefinition>>(); + final grouplessOptions = <OptionDefinition>[];Add import:
import 'dart:collection';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/config/lib/src/config/options.dart(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: indraneel12
PR: serverpod/cli_tools#0
File: :0-0
Timestamp: 2025-10-10T08:50:09.885Z
Learning: PR #71 in serverpod/cli_tools adds support for displaying OptionGroup names in CLI usage output. The implementation does NOT include fallback group names or numbered defaults for anonymous groups - it only displays the group.name when present and shows groupless options first without any header.
🔇 Additional comments (3)
packages/config/lib/src/config/options.dart (3)
44-47: Doc tweak on group header usage — LGTMClear and accurate. Matches the implemented behavior.
842-852: Header insertion logic is correctOnly emits a group header when at least one option in the group is visible; options remain in declared order. Matches the stated requirements.
725-726: Grouping default is safe ConfigParser calls addOptionsToParser with addGroupSeparators:false and remains flat, and CLI tools invoke prepareOptionsForParsing (addGroupSeparators:true) with tests updated for grouped output.
|
@christerswahn Thank you. Btw I haven't focused very much on the Test Cases yet, so there might be some room for improvement in that file. After lunch, I'll see what I can do there (some nitpick comments by the Review Bot) and inform you regardless of any changes by requesting a final review. |
|
Tip For best results, initiate chat on the files or code changes. @indraneel12, you're absolutely right, and I apologize for the confusion in my earlier comments. I've reviewed the actual implementation and can confirm that the Fallback Group Name feature is NOT part of this PR. The implementation in
I've updated my context and added this to my long-term memory to ensure consistent understanding in future interactions. Thank you for catching this and helping me correct my understanding! 🙏 🧠 Learnings used |
Ensures that only one Separator is added per unique Group Name.
- explained the new feature - explained the reason behind a default value
christerswahn
left a comment
christerswahn
left a comment
There was a problem hiding this comment.
Looks good, good job!
2 participants
Resolves #70
Explained by the following Code Demo:
Buggy Output (old behavior; all Group Names vanish):
Expected Output (new behavior; all non-hidden Group Names appear in original order):
Features
usageSummary by CodeRabbit
New Features
Documentation
Tests