feat: Support the carapace CLI completions tool#67
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughModularizes experimental CLI completion: adds a completion module with a generate subcommand, target enum (completely/carapace), usage-representation API, two YAML generators, docs and tests; removes the previous monolithic completion implementation; updates a config MultiOption and an import path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as BetterCommandRunner
participant CC as CompletionCommand
participant CG as CompletionGenerateCommand
participant UR as UsageRepresentation
participant Gen as YAML_Generator
participant Out as Output_Sink
U->>CLI: app completion generate --target {carapace|completely} [--exec-name X] [--file path]
CLI->>CC: dispatch "completion"
CC->>CG: dispatch "generate"
CG->>UR: compile(runner, execNameOverride?)
UR-->>CG: CommandUsage tree
alt target == carapace
CG->>Gen: CarapaceYamlGenerator.generate(out, usage)
else target == completely
CG->>Gen: CompletelyYamlGenerator.generate(out, usage)
end
alt file provided
CG->>Out: open(file), write, flush, close
else
CG->>Out: write to stdout
end
Out-->>U: YAML spec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
christerswahn
force-pushed
the
completion
branch
from
76aab85 to
eda37c6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/config/lib/src/config/options.dart (1)
672-686: Bug: multi-value args break when separator ≠ ',' (usemultiParser.parseand flatten)Current code parses each token with
elementParser.parse, so--opt a;bwithseparator=';'yields a format error. Also remove the cast and use the new field.Apply this diff:
OptionResolution<List<T>>? _resolveNamedArg(final ArgResults? args) { final argOptName = argName; if (argOptName == null || args == null || !args.wasParsed(argOptName)) { return null; } - final multiParser = valueParser as MultiParser<T>; - return OptionResolution( - value: args - .multiOption(argOptName) - .map(multiParser.elementParser.parse) - .toList(), - source: ValueSourceType.arg, - ); + final parser = multiParser; // use field; supports custom separators + final raw = args.multiOption(argOptName); + final parsed = <T>[]; + for (final s in raw) { + // Parses 'a,b' (comma) or 'a;b' (custom sep) and repeated flags. + parsed.addAll(parser.parse(s)); + } + return OptionResolution( + value: parsed, + source: ValueSourceType.arg, + ); }
🧹 Nitpick comments (11)
packages/config/lib/src/config/options.dart (2)
665-666: Ensure defaults are aList<String>(add.toList())
addMultiOptionexpectsdefaultsToasList<String>?; mapping returns anIterable. Convert to a list for broaderargscompatibility.- defaultsTo: defaultValue()?.map(multiParser.elementParser.format), + defaultsTo: defaultValue() + ?.map(multiParser.elementParser.format) + .toList(),
644-649: DeprecateallowedValuesonMultiOptionand steer callers to element-level APIThe override is fine, but it can mislead since list equality is identity-based and not useful here. Marking the getter as deprecated will guide users to
allowedElementValues.- /// [MultiOption] does not properly support [allowedValues], - /// use [allowedElementValues] instead. - @override - List<List<T>>? get allowedValues { + /// [MultiOption] does not properly support [allowedValues], + /// use [allowedElementValues] instead. + @Deprecated('Use allowedElementValues; list-of-list allowedValues is not supported.') + @override + List<List<T>>? get allowedValues { return super.allowedValues; }Additionally, consider renaming the constructor parameter
allowedValuestoallowedElementValues(and deprecate the old name) to avoid confusion with the base class property.Would you like a follow-up patch that introduces a deprecated
allowedValuesparameter alias and a newallowedElementValuesparameter?packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (2)
73-75: Typo in docstring: “gloabl” → “global”.Minor documentation nit.
- /// The gloabl option definitions. + /// The global option definitions.
360-362: Help text: incorrect prefix for abbreviated flag.Should be
-$quietAbbrinstead of--$quietAbbr.- 'Overrides --$quietAbbr, --$quiet.', + 'Overrides -$quietAbbr, --$quiet.',packages/cli_tools/lib/src/better_command_runner/completion/completely_generator.dart (3)
51-55: Potential duplicate option entries when options are inherited + persistent/current.If an option appears in both inherited and current lists, it will be emitted twice. Consider de-dup by (argName, argAbbrev).
- final allOptions = [ - ...inheritedOptions, - ...usage.persistentOptions, - ...usage.options - ]; + final seen = <String>{}; + final allOptions = <OptionDefinition>[ + ...inheritedOptions, + ...usage.persistentOptions, + ...usage.options, + ].where((o) { + final key = '${o.option.argName ?? ''}|${o.option.argAbbrev ?? ''}'; + return seen.add(key); + }).toList();
73-81: First positional completion supported; later ones omitted.Callout is fine for now. Please track support for
argPos > 0as a follow-up.Do you want me to open a small issue to extend positional completion beyond the first argument?
148-155: Quote values to keep YAML safe.Unquoted values containing spaces/colon/hash can break YAML parsing. Safer to always quote.
- for (final value in values) { - out.writeln(' - $value'); - } + for (final value in values) { + out.writeln(' - "$value"'); + }packages/cli_tools/README_completion.md (2)
35-38: Likely filename copy/paste error.You generate
my-command.yamlbut copyexample.yaml.-my-command completion -f my-command.yaml -t carapace -cp example.yaml "${UserConfigDir}/carapace/specs/" +my-command completion -f my-command.yaml -t carapace +cp my-command.yaml "${UserConfigDir}/carapace/specs/"
16-16: Fix markdownlint MD034 (bare URLs).Wrap bare URLs with angle brackets or link text.
-https://carapace.sh/ +<https://carapace.sh/>-https://carapace-sh.github.io/carapace-bin/install.html +<https://carapace-sh.github.io/carapace-bin/install.html>-https://carapace-sh.github.io/carapace-bin/setup.html +<https://carapace-sh.github.io/carapace-bin/setup.html>-https://github.com/bashly-framework/completely +<https://github.com/bashly-framework/completely>Also applies to: 26-26, 61-61, 72-72
packages/cli_tools/lib/src/better_command_runner/completion/carapace_completion.dart (1)
28-33: Optional: add descriptions if available.If we later carry command descriptions in
UsageRepresentation, emitting them here would improve UX in Carapace help.packages/cli_tools/lib/src/better_command_runner/completion/usage_representation.dart (1)
43-53: Return type can be explicit for clarity.Consider returning
CommandUsage<OptionDefinition>to avoid raw types.- static CommandUsage compile( + static CommandUsage<OptionDefinition> compile(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/cli_tools/README_completion.md(3 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/carapace_completion.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completely_generator.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/usage_representation.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion_command.dart(0 hunks)packages/config/lib/src/config/options.dart(2 hunks)
💤 Files with no reviewable changes (1)
- packages/cli_tools/lib/src/better_command_runner/completion_command.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T07:55:17.269Z
Learnt from: christerswahn
PR: serverpod/cli_tools#57
File: packages/config/test/better_command_runner/default_flags_test.dart:1-1
Timestamp: 2025-08-07T07:55:17.269Z
Learning: In the `config` package, `better_command_runner.dart` is intentionally kept as a separate import (`package:config/better_command_runner.dart`) rather than being re-exported through the main `packages/config/lib/config.dart` barrel file. This separation is by design according to the package maintainer christerswahn.
Applied to files:
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart
🪛 markdownlint-cli2 (0.18.1)
packages/cli_tools/README_completion.md
16-16: Bare URL used
(MD034, no-bare-urls)
26-26: Bare URL used
(MD034, no-bare-urls)
61-61: Bare URL used
(MD034, no-bare-urls)
72-72: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (6)
packages/config/lib/src/config/options.dart (1)
618-623: Good addition: exposemultiParserand forward to superPublic field + constructor forwarding improves ergonomics and removes the need for down‑casts elsewhere.
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (1)
8-8: Import path update aligns with new completion module location.Looks good and matches the new
completion/layout.packages/cli_tools/lib/src/better_command_runner/completion/carapace_completion.dart (3)
186-193: LGTM: value sources cover enums, files, and directories.The
$files/$directoriestokens align with Carapace conventions.
150-167: Confirmed — short single-letter keys are allowed for completion.flagVerified: completion.flag keys are the flag "name" (no leading dashes) and may be a single-letter shorthand (examples use "v"); carapace-pflag supports shorthand-only flags.
133-148: Exclusive flag group format confirmed — use bare names (no leading dashes).Carapace expects canonical flag names without dashes (e.g., "quiet", "no-quiet"), so emitting [argName, "no-$argName"] is correct; no change required.
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart (1)
38-81: Solid orchestration and I/O handling.Target dispatch, optional file output, and flushing/closing look correct. Casting
null as Tmatches existing command patterns.
packages/cli_tools/lib/src/better_command_runner/completion/carapace_generator.dart
Show resolved
Hide resolved
packages/cli_tools/lib/src/better_command_runner/completion/usage_representation.dart
Show resolved
Hide resolved
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
I know this is an experimental feature, but I did run into some problems trying to use it for the relic benchmark tool. Perhaps adding some tests would be beneficial at this point?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart(1 hunks)
🔇 Additional comments (8)
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart (8)
1-9: Good modular structure with clean imports.The imports are well-organized and the modular approach with separate generator files follows good separation of concerns.
10-13: LGTM! Clear enumeration of supported completion targets.The enum clearly defines the two supported completion tools with descriptive names.
15-38: Well-structured option definitions with comprehensive help text.The enum-based option definitions are clear and provide good user guidance through help text and abbreviations. The mandatory flag on the target option ensures users must specify a completion target.
40-48: Clean command structure with descriptive metadata.The command class properly extends the base class and provides clear name and description for the completion functionality.
50-65: Solid configuration extraction and usage compilation.The configuration value extraction is clean, and the usage compilation approach provides a good abstraction for generating completion specifications.
56-59: Good defensive programming with clear error message.The null check for the runner with a descriptive exception message is good defensive programming practice.
68-75: Clean generator selection with exhaustive switch.The switch statement properly handles both completion targets and instantiates the appropriate generators. The pattern is clean and extensible.
66-80: File I/O looks correct — no action requiredopenWrite is only used in packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart (lines 66–80). Generators write to the provided IOSink and do not close it; completion_command flushes and await-closes the sink only when a file was opened and intentionally leaves stdout open. No resource-leak or incorrect-close issues found.
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart
Outdated
Show resolved
Hide resolved
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
LGTM 💯 Apart from a few suggestions for the README.
I have successfully used it for the relic benchmark tool.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/cli_tools/README_completion.md (2)
23-23: Fix markdownlint MD034 (bare URLs) by wrapping linksWrap bare URLs with angle brackets or convert to reference links to satisfy MD034 and improve rendering.
Apply this minimal diff:
-https://carapace.sh/ +<https://carapace.sh/> -https://carapace-sh.github.io/carapace-bin/install.html +<https://carapace-sh.github.io/carapace-bin/install.html> -https://carapace-sh.github.io/carapace-bin/setup.html +<https://carapace-sh.github.io/carapace-bin/setup.html> -https://github.com/bashly-framework/completely +<https://github.com/bashly-framework/completely>Also applies to: 33-33, 71-71, 85-85
50-55: Clarify Windows path and copy commandYou mention %APPDATA% but the example uses a POSIX cp. Consider adding a PowerShell equivalent and the full Windows path to reduce friction for users.
Example:
# PowerShell $dest = Join-Path $env:APPDATA 'carapace\specs' New-Item -ItemType Directory -Force -Path $dest | Out-Null Copy-Item .\my-command.yaml -Destination $destpackages/cli_tools/lib/src/better_command_runner/completion/carapace_generator.dart (1)
150-167: Deterministic ordering for completion specs (optional)For reproducible outputs, consider sorting specs by flag name before writing. This reduces churn in diffs across SDK/tooling updates.
packages/cli_tools/lib/src/better_command_runner/completion/completely_generator.dart (1)
148-155: Optional: quote values that contain spacesIf any allowed value includes spaces or YAML-reserved characters, emitting unquoted lines may break parsing. Quoting here is a safe enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/cli_tools/README.md(1 hunks)packages/cli_tools/README_completion.md(2 hunks)packages/cli_tools/lib/src/better_command_runner/completion/carapace_generator.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completely_generator.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_target.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/usage_representation.dart(1 hunks)packages/cli_tools/test/better_command_runner/completion_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/cli_tools/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli_tools/lib/src/better_command_runner/completion/usage_representation.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T13:21:28.476Z
Learnt from: christerswahn
PR: serverpod/cli_tools#67
File: packages/cli_tools/lib/src/better_command_runner/completion/carapace_completion.dart:105-115
Timestamp: 2025-09-22T13:21:28.476Z
Learning: The `!` suffix is supported by Carapace for marking flags as required in the YAML command specification format, as documented in the official Carapace spec at carapace-sh.github.io/carapace-spec/carapace-spec/command/flags.html.
Applied to files:
packages/cli_tools/lib/src/better_command_runner/completion/carapace_generator.dart
📚 Learning: 2025-09-22T13:21:24.881Z
Learnt from: christerswahn
PR: serverpod/cli_tools#67
File: packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart:81-81
Timestamp: 2025-09-22T13:21:24.881Z
Learning: In the BetterCommand framework, using `return null as T` is a consistent pattern for commands that don't return meaningful values, rather than using void return types. The generic T parameter should be maintained even when the command doesn't produce a concrete return value.
Applied to files:
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart
🪛 GitHub Actions: CLI Tools CI
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart
[error] 36-36: The parameter name '_' doesn't match the name 'commandConfig' in the overridden method. Try changing the name to 'commandConfig'. - avoid_renaming_method_parameters. Command 'dart analyze --fatal-infos' failed with 1 issue.
🪛 markdownlint-cli2 (0.18.1)
packages/cli_tools/README_completion.md
23-23: Bare URL used
(MD034, no-bare-urls)
33-33: Bare URL used
(MD034, no-bare-urls)
71-71: Bare URL used
(MD034, no-bare-urls)
85-85: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
packages/cli_tools/lib/src/better_command_runner/completion/completion_target.dart (1)
1-4: LGTM: simple, clear enum for target selectionNames are concise and match the CLI flags/tests. No issues spotted.
packages/cli_tools/test/better_command_runner/completion_test.dart (1)
70-108: Good end-to-end coverage for both generatorsTests validate help wiring, file output, exec-name override, and key YAML fragments for both targets. Nice.
Consider adding two focused cases to catch regressions:
- MultiOption completions for Completely (element allowedValues).
- exclusiveFlags emission for a negatable flag pair (e.g., [mandatory, no-mandatory]) in the Carapace spec.
Also applies to: 149-185, 187-224
| String attributes = ''; | ||
| if (option is! FlagOption) { | ||
| attributes += '='; | ||
| } | ||
| if (option is MultiOption) { | ||
| attributes += '*'; | ||
| } | ||
| if (option.mandatory && option is! FlagOption) { | ||
| attributes += '!'; | ||
| } |
There was a problem hiding this comment.
Quote help text and support required flags ('!') too
- Help text is written unquoted; colons, hashes, or brackets in help can break YAML.
- Required flags (FlagOption.mandatory) aren’t marked with '!'. Carapace supports the '!' suffix for required flags as well, not only options. Based on learnings.
Apply this diff:
@@
-import 'dart:io' show IOSink;
+import 'dart:io' show IOSink;
+import 'dart:convert' show jsonEncode;
@@
static void _declareOption(
@@
- String attributes = '';
- if (option is! FlagOption) {
- attributes += '=';
- }
- if (option is MultiOption) {
- attributes += '*';
- }
- if (option.mandatory && option is! FlagOption) {
- attributes += '!';
- }
+ String attributes = '';
+ if (option is! FlagOption) attributes += '=';
+ if (option is MultiOption) attributes += '*';
+ if (option.mandatory) attributes += '!';
@@
- '${names.join(', ')}$attributes: ${option.helpText ?? ''}',
+ '${names.join(', ')}$attributes: ${_yamlQuote(option.helpText)}',
indentLevel,
);
@@
- '--no-${option.argName}$attributes: ${option.helpText ?? ''}',
+ '--no-${option.argName}$attributes: ${_yamlQuote(option.helpText)}',
indentLevel,
);
@@
}
+
+ static String _yamlQuote(final String? s) => jsonEncode(s ?? '');Also applies to: 116-121, 122-130
🤖 Prompt for AI Agents
In
packages/cli_tools/lib/src/better_command_runner/completion/carapace_generator.dart
around lines 105-114 (and similarly apply the same changes to 116-121 and
122-130), the generated help text is emitted unquoted (which can break YAML when
it contains colons, hashes, or brackets) and required flags are only marked with
'!' for non-FlagOption types; update the generator to always quote/escape the
help text (e.g., wrap in quotes and escape internal quotes/newlines) when
emitting YAML so special characters are safe, and change the attribute-building
logic so that the '!' suffix is added whenever option.mandatory is true
regardless of whether the option is a FlagOption, while preserving the existing
'=' and '*' behavior for non-FlagOption and MultiOption respectively.
| static List<String> _getOptionValues( | ||
| final ConfigOptionBase option, | ||
| ) { | ||
| if (option.allowedValues case final List allowedValues) { | ||
| return allowedValues.map(option.valueParser.format).toList(); | ||
| } | ||
|
|
||
| switch (option.option) { | ||
| case EnumOption(): | ||
| final enumParser = option.option.valueParser as EnumParser; | ||
| return enumParser.enumValues.map(enumParser.format).toList(); | ||
| case FileOption(): | ||
| return ['<file>']; | ||
| case DirOption(): | ||
| return ['<directory>']; | ||
| default: | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add MultiOption element completions to match Carapace parity
Completely currently ignores MultiOption.allowedElementValues, so users won’t get element suggestions for multi-valued options. Carapace generator handles this. Bring parity by formatting element values via the MultiOption’s element parser.
Apply this diff:
static List<String> _getOptionValues(
final ConfigOptionBase option,
) {
- if (option.allowedValues case final List allowedValues) {
+ if (option case final MultiOption multi) {
+ if (multi.allowedElementValues case final List allowedValues) {
+ return allowedValues
+ .map<String>(multi.multiParser.elementParser.format)
+ .toList();
+ }
+ } else if (option.allowedValues case final List allowedValues) {
return allowedValues.map(option.valueParser.format).toList();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static List<String> _getOptionValues( | |
| final ConfigOptionBase option, | |
| ) { | |
| if (option.allowedValues case final List allowedValues) { | |
| return allowedValues.map(option.valueParser.format).toList(); | |
| } | |
| switch (option.option) { | |
| case EnumOption(): | |
| final enumParser = option.option.valueParser as EnumParser; | |
| return enumParser.enumValues.map(enumParser.format).toList(); | |
| case FileOption(): | |
| return ['<file>']; | |
| case DirOption(): | |
| return ['<directory>']; | |
| default: | |
| return []; | |
| } | |
| } | |
| static List<String> _getOptionValues( | |
| final ConfigOptionBase option, | |
| ) { | |
| if (option case final MultiOption multi) { | |
| if (multi.allowedElementValues case final List allowedValues) { | |
| return allowedValues | |
| .map<String>(multi.multiParser.elementParser.format) | |
| .toList(); | |
| } | |
| } else if (option.allowedValues case final List allowedValues) { | |
| return allowedValues.map(option.valueParser.format).toList(); | |
| } | |
| switch (option.option) { | |
| case EnumOption(): | |
| final enumParser = option.option.valueParser as EnumParser; | |
| return enumParser.enumValues.map(enumParser.format).toList(); | |
| case FileOption(): | |
| return ['<file>']; | |
| case DirOption(): | |
| return ['<directory>']; | |
| default: | |
| return []; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
packages/cli_tools/lib/src/better_command_runner/completion/completely_generator.dart
around lines 128 to 146, the function _getOptionValues ignores
MultiOption.allowedElementValues and therefore omits element completions for
multi-valued options; update the function to handle MultiOption by: when option
is a MultiOption (or when option.allowedElementValues is present), obtain the
element parser (option.option.elementParser or similar), map
option.allowedElementValues through that parser's format method and return the
formatted list; if allowedElementValues is absent but option.option is
MultiOption, fall back to returning a placeholder like ['<value>'] consistent
with FileOption/DirOption handling.
packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart
Outdated
Show resolved
Hide resolved
2 participants
Adds a Yaml specification generator for another command line completion tool,
carapace.https://carapace.sh/
The benefit with Carapace, compared with Completely, is that it supports a lot of shells, including Bash, ZSH, Fish, Elvish, Powershell, Cmd, and more.
The drawback is that
carapacerequires end users to install it, rather than just distributing a pre-baked script with the command.Example of a carapace Yaml spec file:
Summary by CodeRabbit
New Features
Documentation
Tests