refactor(cli_tools)!: Renamed 'target' option to 'tool' #83
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR renames the completion API concept from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/cli_tools/example/command_completion_example.dart(1 hunks)packages/cli_tools/example/completion_script_carapace.dart(1 hunks)packages/cli_tools/example/completion_script_completely.dart(4 hunks)packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(1 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion.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_embed_command.dart(5 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart(3 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart(4 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_target.dart(0 hunks)packages/cli_tools/lib/src/better_command_runner/completion/completion_tool.dart(1 hunks)packages/cli_tools/test/better_command_runner/completion_test.dart(8 hunks)
💤 Files with no reviewable changes (1)
- packages/cli_tools/lib/src/better_command_runner/completion/completion_target.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/completion/completion.dartpackages/cli_tools/lib/src/better_command_runner/better_command_runner.dart
🔇 Additional comments (20)
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (1)
9-9: LGTM! Clean import update.The import path correctly references the new
completion_tool.dartfile while maintaining theCompletionScriptexport. This aligns with the broader refactoring effort.packages/cli_tools/lib/src/better_command_runner/completion/completion.dart (1)
1-1: LGTM! Export correctly updated.The export now references
completion_tool.dart, maintaining the public API surface while aligning with the renamed module.packages/cli_tools/lib/src/better_command_runner/completion/completion_tool.dart (1)
1-6: LGTM! Clean API definition.The new
CompletionToolenum andCompletionScripttypedef are well-defined. The rename fromtargettotoolprovides clearer semantics about the purpose of this option.packages/cli_tools/lib/src/better_command_runner/completion/completion_command.dart (2)
9-9: LGTM! Import correctly updated.The import now references the new
completion_tool.dartmodule.
12-23: LGTM! Option definition properly refactored.The rename from
targetOptiontotoolOptionis consistent throughout, with appropriate updates to the argument name and help text. The preservation of the-tabbreviation minimizes the impact of this breaking change on users who rely on the shorthand.packages/cli_tools/example/completion_script_completely.dart (3)
4-4: LGTM! Import updated correctly.The import now shows
CompletionToolinstead of the oldCompletionTarget.
63-100: LGTM! Embedded completion script updated consistently.The bash completion script has been properly regenerated to reference
--toolinstead of--targetthroughout all completion cases. This ensures users get accurate completions for the renamed option.
117-117: LGTM! Record field renamed correctly.The record now uses
tool: CompletionTool.completelyinstead of the oldtarget:field name, consistent with the API refactor.packages/cli_tools/example/completion_script_carapace.dart (3)
4-4: LGTM! Import updated correctly.The import now shows
CompletionToolinstead ofCompletionTarget.
10-34: LGTM! Embedded carapace specification updated consistently.The YAML specification has been properly regenerated with all references to
targetreplaced withtool, including flag definitions and completion mappings. This ensures carapace will provide accurate completions for the renamed option.
42-42: LGTM! Record field renamed correctly.The record now uses
tool: CompletionTool.carapace, matching the new API surface.packages/cli_tools/lib/src/better_command_runner/completion/completion_generate_command.dart (4)
10-10: LGTM! Import updated correctly.The import now references
completion_tool.dart.
14-14: LGTM! Enum value renamed consistently.The enum value is now
tooland correctly referencesCompletionOptions.toolOption.
41-41: LGTM! Variable and config access updated.The variable is now named
tooland retrieves the correct option value.
58-65: LGTM! Switch statement updated to use new enum.The switch cases now correctly reference
CompletionTool.completelyandCompletionTool.carapace.packages/cli_tools/test/better_command_runner/completion_test.dart (3)
93-93: LGTM! Test assertions updated correctly.The test expectations now verify that generated specifications reference
--toolinstead of--target, ensuring the refactor is working as intended.Also applies to: 129-129, 163-163, 168-168
235-235: LGTM! Embedded script generation tests updated.The tests verify that embedded scripts are generated with the correct import (
CompletionTool) and record field name (tool:), ensuring consistency across the public API.Also applies to: 250-250, 290-290, 302-302
337-337: LGTM! Test data uses new enum correctly.The test data now uses
CompletionTool.completelyandCompletionTool.carapace, matching the refactored API.Also applies to: 341-341
packages/cli_tools/lib/src/better_command_runner/completion/completion_embed_command.dart (1)
13-13: LGTM! Clean and consistent refactor from target to tool.The rename is systematic and complete throughout this file:
- Enum value correctly references
CompletionOptions.toolOption- File naming template updated to use
<tool>placeholder- Runtime configuration reads use
toolconsistently- Generated code template correctly imports
CompletionTooland usestoolin all referencesAlso applies to: 25-25, 64-67, 98-98, 116-116, 122-124
packages/cli_tools/lib/src/better_command_runner/completion/completion_install_command.dart (1)
10-10: LGTM! Thorough and consistent refactor.The refactor is complete and well-executed:
- Import updated to
completion_tool.dart- Type signatures updated (
Map<CompletionTool, String>)- Constructor correctly maps
e.toolinstead ofe.target- All runtime references use
toolconsistently- Error messages use the new "tool" terminology
- Switch expressions correctly reference
CompletionTool.completelyandCompletionTool.carapaceAlso applies to: 13-13, 30-30, 35-35, 48-48, 59-61, 67-85
nielsenko
left a comment
nielsenko
left a comment
There was a problem hiding this comment.
I agree, and the down stream dependencies are still few. Go for it 🐰
Had just one comment on a comment.. Nittiest of nits. Feel free to disregard.
Renamed the
completionsubcommands' --target option to --tool. "Tool" is more intuitive since it is actually the tool that is selected, and "target" could be confusing when also specifying file/dir to write to.Although this is a breaking change since the option name changes, it is good to change while this is a recently added feature, and most uses use the shorthand
-twhich is unchanged.Summary by CodeRabbit
Refactor
Documentation
Tests