feat: builtin options with plugin system - #155
Conversation
|
Warning Rate limit exceeded@kazupon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughA plugin system was introduced to the Gunshi CLI. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant PluginContext
participant Plugins
participant CommandParser
User->>CLI: Run CLI with argv
CLI->>PluginContext: Create instance
CLI->>Plugins: Apply each plugin to PluginContext
Plugins->>PluginContext: Register global options
CLI->>CommandParser: Parse argv with PluginContext.globalOptions
CommandParser->>CLI: Return parsed arguments
CLI->>CLI: Execute command with merged options
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/gunshi/src/plugins/dryrun.ts (1)
11-15: Plugin structure looks good, placeholder implementation noted.The plugin follows the correct
Plugintype signature and integrates properly with the plugin system. The placeholder implementation is understandable for initial plugin architecture setup.Note: The pipeline warning about the unused file is expected since this is a placeholder plugin not yet integrated into the CLI flow. Would you like me to help implement the dry-run functionality in a future iteration?
packages/gunshi/src/plugins/completion.ts (1)
11-15: Consistent plugin structure established.The completion plugin follows the same correct pattern as other plugins in the system. The placeholder implementation aligns with the incremental approach to building the plugin architecture.
Like the dry-run plugin, this is appropriately flagged as unused since it's a placeholder. Would you like assistance implementing shell completion functionality in the future?
packages/gunshi/src/plugin.ts (2)
32-34: Remove the unnecessary constructor.The constructor is currently empty with only a TODO comment. Since no initialization logic is present, it can be removed entirely.
Apply this diff to remove the unnecessary constructor:
- constructor() { - // TODO: - }🧰 Tools
🪛 Biome (1.9.4)
[error] 32-34: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
48-49: Complete the plugin context hooks implementation.The TODO comment indicates that additional hooks are planned but not yet implemented. Consider defining the interface for these hooks to establish the plugin architecture more completely.
Would you like me to help design the additional plugin hooks interface or open an issue to track this implementation?
packages/gunshi/src/cli.ts (1)
39-40: Consider making the plugin list configurable.The current implementation hardcodes the
builtinsplugin. Consider making the plugin list configurable to support future extensibility.Consider this approach for better extensibility:
+ const plugins = cliOptions.plugins || [builtins] - for (const plugin of [builtins]) { + for (const plugin of plugins) { await plugin()(pluginContext) }This would require updating the
CliOptionstype to include an optionalpluginsfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/gunshi/src/cli.ts(5 hunks)packages/gunshi/src/plugin.ts(1 hunks)packages/gunshi/src/plugins/builtins.ts(1 hunks)packages/gunshi/src/plugins/completion.ts(1 hunks)packages/gunshi/src/plugins/dryrun.ts(1 hunks)packages/gunshi/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/gunshi/src/plugins/dryrun.ts (1)
packages/gunshi/src/plugin.ts (1)
Plugin(24-24)
packages/gunshi/src/plugins/completion.ts (1)
packages/gunshi/src/plugin.ts (1)
Plugin(24-24)
packages/gunshi/src/plugins/builtins.ts (2)
packages/gunshi/src/plugin.ts (1)
Plugin(24-24)packages/gunshi/src/constants.ts (1)
COMMON_ARGS(37-48)
packages/gunshi/src/plugin.ts (1)
packages/gunshi/src/types.ts (1)
Awaitable(10-10)
🪛 GitHub Actions: CI
packages/gunshi/src/plugins/dryrun.ts
[warning] 1-1: Unused file detected by knip.
packages/gunshi/src/plugins/completion.ts
[warning] 1-1: Unused file detected by knip.
🪛 Biome (1.9.4)
packages/gunshi/src/plugin.ts
[error] 32-34: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (4)
packages/gunshi/src/types.ts (1)
10-10: Essential type export for plugin system.Exporting
Awaitable<T>enables the plugin system to use this type definition across modules, which is necessary for thePlugintype definition inpackages/gunshi/src/plugin.ts.packages/gunshi/src/plugins/builtins.ts (1)
13-19: Excellent implementation of plugin architecture objectives.This plugin successfully converts built-in options (
--helpand--versionfromCOMMON_ARGS) into the new plugin system, directly fulfilling the PR's main objective. The implementation correctly:
- Follows the established
Plugintype pattern- Iterates through all common arguments using
Object.entries- Registers each as a global option via
ctx.addGlobalOption- Enables modular management of built-in options
This change enables the future work mentioned in the PR description where users can opt out of built-in options through CLI settings.
packages/gunshi/src/plugin.ts (1)
40-42: Good defensive programming practice.Creating a new Map instance in the getter prevents external code from mutating the internal state, which is excellent for maintaining data integrity.
packages/gunshi/src/cli.ts (1)
61-61: Good variable renaming for clarity.Renaming
ctxtocommandContextimproves code readability and reduces potential confusion with the newPluginContext.Also applies to: 75-75, 81-81, 87-87, 95-95, 99-99
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Deploying gunshi with
|
| Latest commit: |
7305b4b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://92d672f7.gunshi.pages.dev |
| Branch Preview URL: | https://feat-plugin-system-1.gunshi.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/gunshi/src/plugins/globals.ts (1)
13-17: Consider adding error handling for robustness.The function correctly implements the plugin pattern for registering global options. However, it doesn't handle potential errors from
ctx.addGlobalOption(), which can throw errors for invalid option names or duplicate registrations.While this might be acceptable for built-in options that should be well-defined, consider adding basic error handling for better robustness:
export default function globals(ctx: PluginContext) { for (const [name, schema] of Object.entries(COMMON_ARGS)) { - ctx.addGlobalOption(name, schema) + try { + ctx.addGlobalOption(name, schema) + } catch (error) { + throw new Error(`Failed to register global option '${name}': ${error.message}`) + } } }Alternatively, if you prefer to keep it simple since these are built-in options, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/gunshi/src/__snapshots__/cli.test.ts.snapis excluded by!**/*.snappackages/gunshi/src/__snapshots__/generator.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
knip.config.ts(1 hunks)packages/gunshi/src/cli.ts(5 hunks)packages/gunshi/src/plugins/completion.ts(1 hunks)packages/gunshi/src/plugins/dryrun.ts(1 hunks)packages/gunshi/src/plugins/globals.ts(1 hunks)packages/gunshi/src/plugins/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- knip.config.ts
- packages/gunshi/src/plugins/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/gunshi/src/plugins/completion.ts
- packages/gunshi/src/plugins/dryrun.ts
- packages/gunshi/src/cli.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/gunshi/src/plugins/globals.ts (2)
packages/gunshi/src/plugin.ts (1)
PluginContext(29-55)packages/gunshi/src/constants.ts (1)
COMMON_ARGS(37-48)
🪛 GitHub Check: Spell check with Typos
packages/gunshi/src/plugins/globals.ts
[warning] 11-11:
"globa" should be "global".
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Type Checking on Node.js 20
- GitHub Check: Build on Node.js 20
- GitHub Check: Lint on Node.js 20
- GitHub Check: Spell check with Typos
- GitHub Check: release
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/gunshi/src/plugins/globals.ts (1)
6-17: LGTM! Clean plugin implementation.The overall design effectively converts built-in options into a plugin architecture. The function cleanly separates global options registration from the main CLI logic and follows the established plugin pattern. The integration with
COMMON_ARGSandPluginContextis well-structured.🧰 Tools
🪛 GitHub Check: Spell check with Typos
[warning] 11-11:
"globa" should be "global".
1 participant
Description
This PR is the first step toward adopting a plugin architecture for gunshi.
Built-in options such as
--helpand--versionwill be implemented as plugins.Taking #69 and #70 into consideration, I will make it possible to opt out of these options via
clioptions in the near future.Linked Issues
#2
Additional context
Summary by CodeRabbit