feat(plugin-i18n): registerGlobalOptionResources extension - #343
Conversation
WalkthroughAdds a global-option localization mechanism to the i18n plugin (registering per-option locale resources), exposes Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant I18n as i18n Plugin
participant Adapter as Adapter/Resource Store
App->>I18n: initialize extension(ctx)
I18n->>I18n: derive builtinGlobalOptions ← COMMON_ARGS
I18n->>I18n: register built-in global option resources
I18n-->>App: extension exposed (includes registerGlobalOptionResources)
Note right of App: At runtime
App->>I18n: loadResource(locale)
I18n->>Adapter: request base resources
Adapter-->>I18n: base resources
alt global option resources exist
I18n->>I18n: collect per-option resources for locale
I18n->>Adapter: merge option-specific locale strings into resources
end
I18n-->>App: merged resources returned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
@gunshi/bone
@gunshi/definition
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
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: 2
🧹 Nitpick comments (1)
packages/plugin-i18n/src/index.ts (1)
294-303: Consider renaming function for clarity.The function now conditionally maps keys based on whether they are global options (using
resolveArgKey) or built-in resources (usingresolveBuiltInKey). The current namemapResourceWithBuiltinKeyis misleading since it doesn't exclusively map built-in keys anymore.Consider renaming to better reflect its dual purpose:
-function mapResourceWithBuiltinKey( +function mapResourceKeys( resource: Record<string, string>, ctx: CommandContext, globalOptions: string[] ): Record<string, string> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
e2e/fixture/advanced/internationalization/loading/english-help.snapis excluded by!**/*.snape2e/fixture/advanced/internationalization/loading/japanese-help.snapis excluded by!**/*.snape2e/fixture/advanced/internationalization/sub-command/english-create-help.snapis excluded by!**/*.snape2e/fixture/advanced/internationalization/sub-command/english-help.snapis excluded by!**/*.snape2e/fixture/advanced/internationalization/sub-command/japanese-create-help.snapis excluded by!**/*.snape2e/fixture/advanced/internationalization/sub-command/japanese-help.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/gunshi/src/types.ts(1 hunks)packages/plugin-i18n/README.md(1 hunks)packages/plugin-i18n/src/index.test.ts(0 hunks)packages/plugin-i18n/src/index.ts(6 hunks)packages/plugin-i18n/src/types.ts(1 hunks)packages/shared/src/utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/plugin-i18n/src/index.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use ES modules throughout the codebase
Follow existing code style (enforced by ESLint and Prettier)
Files:
packages/plugin-i18n/src/types.tspackages/plugin-i18n/src/index.tspackages/gunshi/src/types.tspackages/shared/src/utils.ts
packages/gunshi/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
packages/gunshi/src/**/*.ts: All source code is in TypeScript with strict mode enabled
Type safety is a core feature - maintain strict TypeScript types throughout
Files:
packages/gunshi/src/types.ts
🧬 Code graph analysis (2)
packages/plugin-i18n/src/index.ts (5)
packages/gunshi/src/plugin/core.ts (1)
plugin(390-436)packages/shared/src/utils.ts (2)
resolveArgKey(52-57)resolveBuiltInKey(34-38)packages/shared/src/constants.ts (1)
COMMON_ARGS(33-44)packages/gunshi/src/types.ts (1)
CommandContext(361-451)packages/gunshi/src/plugin/context.ts (1)
globalOptions(168-170)
packages/shared/src/utils.ts (2)
packages/plugin-i18n/src/index.ts (1)
resolveBuiltInKey(62-62)packages/shared/src/types.ts (2)
CommandBuiltinResourceKeys(68-68)GenerateNamespacedKey(55-58)
🪛 LanguageTool
packages/plugin-i18n/README.md
[grammar] ~343-~343: Ensure spelling is correct
Context: ...tion, you can install resource of it at exntesion or onExtension hook ## 📝 Resource K...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on Node.js 20
🔇 Additional comments (9)
packages/plugin-i18n/src/types.ts (1)
83-89: LGTM! Clear public API extension for global option resources.The new
registerGlobalOptionResourcesmethod extends the I18nExtension interface to support registering locale-specific resources for global options. The signature is well-defined, and the documentation clearly explains its purpose.packages/plugin-i18n/src/index.ts (7)
40-40: LGTM! Import required for global option resources.The
COMMON_ARGSimport is necessary for setting up built-in global option resources (help and version options).
104-104: LGTM! Appropriate data structure for global option resources.The
globalOptionResourcesmap stores locale-specific resources for each global option, which is accessed and managed by the helper functions below.
163-167: LGTM! Updated resource mapping to handle global options.The
setResourcefunction now passes the list of global options tomapResourceWithBuiltinKey, enabling conditional key resolution based on whether a key represents a global option (usingresolveArgKey) or a built-in resource (usingresolveBuiltInKey).
212-219: LGTM! Global option resources are correctly augmented during resource loading.The logic iterates through registered global options and injects locale-specific resources, falling back to built-in resources if a locale-specific value is not available. This ensures that global options are properly localized for each command.
230-231: LGTM! Correctly exposes the new API method.The extension now returns
registerGlobalOptionResources, making it available to consumers viactx.extensions['g:i18n'].registerGlobalOptionResources.
111-124: Verification confirmed: async extension factories are supported.The plugin system explicitly awaits extension factories (line 231 in
core.test.ts:await extension.factory(...)), and the type system confirms they return Promises (usingAwaited<ReturnType<...>>patterns at lines 350, 419-420, 496). The changes inpackages/plugin-i18n/src/index.tsare compatible with the plugin architecture.
169-187: DefaultResource contains all COMMON_ARGS keys—no issues found.Verification confirms that
DefaultResource(frompackages/resources/locales/en-US.json) contains bothhelpandversionkeys that correspond toCOMMON_ARGS. Both built-in locale resources (en-US and ja-JP) include these keys, so accessing them at line 176 is safe.The cast
(DefaultResource as Record<string, string>)[globalOption]will not produceundefinedvalues for the built-in resources.packages/shared/src/utils.ts (1)
34-36: Type constraint narrowing is correct and verified across all call sites.The default generic parameter has been narrowed from
CommandBuiltinArgsKeys | CommandBuiltinResourceKeystoCommandBuiltinResourceKeys. This change is correct:
- All direct call sites use resource key literals (test files, plugin-renderer)
- The critical usage in plugin-i18n conditionally routes args keys through
resolveArgKey()instead, ensuring only resource keys reachresolveBuiltInKey()CommandBuiltinArgsKeysis not imported in utils.ts and only exists in the union type definition- The constraint accurately reflects the function's actual semantic purpose
No breaking changes detected in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
knip.config.ts(1 hunks)packages/docs/src/guide/plugin/guidelines.md(1 hunks)packages/plugin-i18n/README.md(1 hunks)packages/plugin-i18n/package.json(1 hunks)packages/plugin-i18n/src/index.test.ts(3 hunks)packages/plugin-i18n/src/index.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/plugin-i18n/README.md
- packages/plugin-i18n/src/index.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use ES modules throughout the codebase
Follow existing code style (enforced by ESLint and Prettier)
Files:
knip.config.tspackages/plugin-i18n/src/index.ts
🧬 Code graph analysis (1)
packages/plugin-i18n/src/index.ts (4)
packages/shared/src/constants.ts (1)
COMMON_ARGS(33-44)packages/shared/src/utils.ts (2)
resolveArgKey(52-57)resolveBuiltInKey(34-38)packages/gunshi/src/types.ts (1)
CommandContext(361-451)packages/gunshi/src/plugin/context.ts (1)
globalOptions(168-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on Node.js 20
🔇 Additional comments (10)
packages/docs/src/guide/plugin/guidelines.md (1)
59-60: Inconsistent example in scoped package naming patterns.The example
@feature/gunshi/pluginon line 60 does not match the documented scoped package formats. According to the patterns defined on line 59:
@{org}/gunshi-plugin-{feature}→ would match@feature/gunshi-plugin@{org}/gunshi-plugin→ would match@feature/gunshi-pluginThe provided example uses a different pattern (
/gunshi/pluginwith a slash instead of hyphen), which deviates from the naming conventions being described.Please clarify whether:
@feature/gunshi/pluginis an intentional undocumented format and should be added to the patterns list, or- The example should be corrected to match one of the documented patterns (e.g.,
@feature/gunshi-plugin)knip.config.ts (1)
15-17: LGTM! Appropriate handling of optional peer dependency.The
ignoreDependenciesrule correctly prevents Knip from flagging@gunshi/plugin-globalas unused. This is appropriate since it's an optional peer dependency used in development/testing.packages/plugin-i18n/package.json (1)
74-76: LGTM! DevDependencies correctly added for testing.The three devDependencies are appropriately added to support testing the new global option resources functionality:
@gunshi/boneand@gunshi/plugin-rendererprovide test infrastructure@gunshi/plugin-globalfollows the standard pattern of adding an optional peer dependency as a devDependency to ensure it's available during testingAll use the correct
workspace:*protocol and maintain alphabetical ordering.packages/plugin-i18n/src/index.ts (7)
40-40: LGTM!The import of
COMMON_ARGSis appropriate for initializing built-in global options.
104-105: LGTM!The Map structure for storing global option resources is appropriately typed and initialized.
117-119: Verify if preventing updates to registered options is intentional.The early return prevents updates to already-registered global option resources. If a plugin or user code calls
registerGlobalOptionResourceswith the same option name but different resources, the new resources will be silently ignored.Is this the intended behavior, or should updates be allowed?
142-144: LGTM!The helper function correctly returns all registered global option keys as an array.
174-178: LGTM!The updated call to
mapResourceWithBuiltinKeycorrectly passesgetGlobalOptions()to enable proper key mapping for global options.
180-197: LGTM with noted typo.The logic for setting up and registering built-in global option resources is correct. The variable name typo at line 181 (
buildinGlobalOptionResources) should be corrected as noted in the previous comment.
305-314: LGTM!The updated signature and logic correctly differentiate global options (mapped using
resolveArgKey) from other built-in keys (mapped usingresolveBuiltInKey).
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/plugin-i18n/src/index.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use ES modules throughout the codebase
Follow existing code style (enforced by ESLint and Prettier)
Files:
packages/plugin-i18n/src/index.ts
🧬 Code graph analysis (1)
packages/plugin-i18n/src/index.ts (5)
packages/shared/src/constants.ts (1)
COMMON_ARGS(33-44)packages/gunshi/src/plugin/core.ts (1)
plugin(390-436)packages/shared/src/utils.ts (2)
resolveArgKey(52-57)resolveBuiltInKey(34-38)packages/gunshi/src/types.ts (1)
CommandContext(361-451)packages/gunshi/src/plugin/context.ts (1)
globalOptions(168-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test on Node.js 20
- GitHub Check: release
- GitHub Check: release
🔇 Additional comments (9)
packages/plugin-i18n/src/index.ts (9)
40-40: LGTM!The import of
COMMON_ARGSis correctly added and used to derive built-in global options.
101-102: LGTM!The variable naming is correct and consistent with the codebase conventions (e.g.,
builtinResources). The previous typo concern has been addressed.
105-105: LGTM!The type signature correctly models per-option, per-locale resource storage.
142-144: LGTM!The helper correctly returns a snapshot of registered global option keys.
174-178: LGTM!The integration of global options into resource mapping is correct and aligns with the feature's objective to treat global options as arguments.
180-197: LGTM!The setup logic correctly builds locale-specific resources for each built-in global option, ensuring at minimum a
DEFAULT_LOCALEfallback exists.
243-243: LGTM!The public API correctly exposes
registerGlobalOptionResources, enabling external plugins to register global option localization.
306-315: LGTM, but verify ctx.name is defined at call sites.The updated logic correctly differentiates global options (mapped to arg keys) from other built-in keys. However, note that
ctx.nameat line 312 can be undefined per theCommandContexttype. Ensure all call sites (line 176) occur in contexts wherectx.nameis guaranteed to be defined.
113-140: Original review comment is incorrect.The
resolveArgKeyfunction explicitly declaresnameas an optional parameter (name?: string) and handles undefined values in its implementation:${name ?${name}${BUILT_IN_KEY_SEPARATOR}: ''}...Whenctx.nameis undefined, the function correctly omits the name prefix and returns a valid key format. No guard or defensive check is needed at line 129.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/plugin-i18n/src/index.ts (1)
113-140: Consider logging when duplicate registration is silently ignored.At lines 117-119, the function silently returns early if the option is already registered. This prevents resource updates and might hide mistakes where a developer attempts to modify or re-register global option resources. Consider logging a warning or throwing an error to make this behavior explicit.
Apply this diff to add a warning:
if (globalOptionResources.has(option)) { + console.warn(`Global option "${option}" is already registered. Skipping duplicate registration.`) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugin-i18n/src/index.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use ES modules throughout the codebase
Follow existing code style (enforced by ESLint and Prettier)
Files:
packages/plugin-i18n/src/index.ts
🧬 Code graph analysis (1)
packages/plugin-i18n/src/index.ts (3)
packages/shared/src/constants.ts (1)
COMMON_ARGS(33-44)packages/shared/src/utils.ts (3)
namespacedId(105-109)resolveArgKey(52-57)resolveBuiltInKey(34-38)packages/gunshi/src/types.ts (1)
CommandContext(361-451)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on Node.js 20
🔇 Additional comments (6)
packages/plugin-i18n/src/index.ts (6)
40-40: LGTM!The import of
COMMON_ARGSis correctly added to support the global option resources feature.
101-105: LGTM!The variable declarations correctly establish storage for built-in global options and global option resources. The typo from the previous review has been addressed.
174-197: LGTM!The setup of built-in global option resources is well-structured. The logic correctly:
- Initializes each built-in option with the default locale (en-US) from
DefaultResource- Enriches with additional locales from
builtinResources- Registers all options via
registerGlobalOptionResourcesThis ensures that built-in global options always have a
DEFAULT_LOCALEfallback.
224-234: LGTM! Defensive fallback properly implemented.The fallback chain at lines 228-232 now includes:
- Requested locale (
localeStr)- Default locale (
DEFAULT_LOCALE)- First available locale (
Object.values(optionResource)[0])- Empty string fallback
This addresses the previous review concern about potentially undefined values when externally registered options lack the default locale.
310-319: LGTM!The signature update correctly adds
globalOptionsparameter to conditionally route keys to the appropriate resolver:
- Global options use
resolveArgKey(command-namespaced)- Built-in keys use
resolveBuiltInKey(built-in prefix)This properly differentiates between global option resources and other built-in resources.
243-248: LGTM!The extension correctly exposes
registerGlobalOptionResourcesas part of the public API, enabling external code to register custom global option resources.
1 participant
Description
Linked Issues
Additional context
Summary by CodeRabbit
New Features
Documentation
Tests
Chores