feat(plugin-dryrun): implement dry-run extension#568
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete dry-run plugin for the Gunshi CLI framework. It enables commands to selectively skip side-effectful operations via a ChangesDry-Run Plugin Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying gunshi with
|
| Latest commit: |
7e1b8fe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c179c9e1.gunshi.pages.dev |
| Branch Preview URL: | https://feat-plugin-dryrun.gunshi.pages.dev |
@gunshi/bone
@gunshi/combinators
@gunshi/definition
@gunshi/docs
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/plugin-dryrun/src/index.ts (3)
195-200: 💤 Low valueDocument the precedence of
resolveoverresult.While the type definition (
DryRunRunResult) is a union ensuring only one field exists, at runtime an object could contain bothresolveandresult. The code prioritizesresolve(line 195-196) overresult(line 198-199), but this precedence is not documented in the JSDoc or comments.Consider adding a brief comment explaining this behavior, or asserting mutual exclusivity if stricter validation is desired.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugin-dryrun/src/index.ts` around lines 195 - 200, Document that when both fields are present at runtime the function will prefer the resolve path: add a brief JSDoc or inline comment in packages/plugin-dryrun/src/index.ts near the branch handling options (the block checking 'resolve' in options and 'result' in options) stating that resolve takes precedence over result, and optionally add a runtime assertion or validation (e.g., in the same function that handles DryRunRunResult) to enforce mutual exclusivity if stricter behavior is desired; reference the options object and the DryRunRunResult union in the comment so readers know which symbols/types the precedence applies to.
204-218: ⚖️ Poor tradeoffSame defensive consideration applies to
resolveWrapResult.Like
resolveRunResult, this function returnsundefined as Rwhen no fallback is provided (lines 209, 217). The same type-safety considerations and defensive programming suggestions apply here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugin-dryrun/src/index.ts` around lines 204 - 218, The resolveWrapResult function currently returns undefined cast to R when no options or no fallback is provided, which is unsafe; update resolveWrapResult (and its signature if needed) to mirror the defensive fix used for resolveRunResult: when options is missing or neither resolve nor result are present, either return a well-typed explicit fallback (e.g., a provided default value or Promise.resolve(default)) or throw a clear error; ensure you reference and adjust the DryRunMessage/DryRunWrapResult types if necessary so the return type no longer relies on casting undefined to R and callers cannot receive an unexpected undefined.
189-202: ⚖️ Poor tradeoffConsider defensive guard for non-void fallback resolution.
The function returns
undefined as Rwhen no fallback is provided (lines 193, 201). While the public API typeDryRunOptionsArgenforces that non-void operations must provideresultorresolve, the internal parameter typePartial<DryRunRunResult<R>>allows both fields to be absent.If a caller bypasses type checking (e.g., via
as any), or if there's a generic instantiation edge case, a non-void task could receiveundefinedinstead of a proper value, breaking type safety at runtime.🛡️ Consider adding a runtime guard
function resolveRunResult<R>( options?: DryRunMessage & Partial<DryRunRunResult<R>> ): Awaitable<R> { if (!options) { return undefined as R } if ('resolve' in options && options.resolve) { return options.resolve() } if ('result' in options) { return options.result as R } + // This path should be unreachable for non-void R due to DryRunOptionsArg, + // but provides a runtime safeguard if type constraints are bypassed return undefined as R }Note: This is a defense-in-depth measure; the current implementation is type-safe when used correctly through the public API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugin-dryrun/src/index.ts` around lines 189 - 202, The resolveRunResult function currently returns undefined as R in two fall-through cases which can leak undefined at runtime; change those fall-through returns to throw a clear runtime error (e.g., "DryRun: missing result/resolve for non-void run") so callers cannot receive undefined when no result/resolve was provided; update the function resolveRunResult<R>(options?: DryRunMessage & Partial<DryRunRunResult<R>>) to throw when options is falsy or when neither 'resolve' nor 'result' is present, leaving the existing branches for options.resolve() and options.result intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/plugin-dryrun/src/index.ts`:
- Around line 195-200: Document that when both fields are present at runtime the
function will prefer the resolve path: add a brief JSDoc or inline comment in
packages/plugin-dryrun/src/index.ts near the branch handling options (the block
checking 'resolve' in options and 'result' in options) stating that resolve
takes precedence over result, and optionally add a runtime assertion or
validation (e.g., in the same function that handles DryRunRunResult) to enforce
mutual exclusivity if stricter behavior is desired; reference the options object
and the DryRunRunResult union in the comment so readers know which symbols/types
the precedence applies to.
- Around line 204-218: The resolveWrapResult function currently returns
undefined cast to R when no options or no fallback is provided, which is unsafe;
update resolveWrapResult (and its signature if needed) to mirror the defensive
fix used for resolveRunResult: when options is missing or neither resolve nor
result are present, either return a well-typed explicit fallback (e.g., a
provided default value or Promise.resolve(default)) or throw a clear error;
ensure you reference and adjust the DryRunMessage/DryRunWrapResult types if
necessary so the return type no longer relies on casting undefined to R and
callers cannot receive an unexpected undefined.
- Around line 189-202: The resolveRunResult function currently returns undefined
as R in two fall-through cases which can leak undefined at runtime; change those
fall-through returns to throw a clear runtime error (e.g., "DryRun: missing
result/resolve for non-void run") so callers cannot receive undefined when no
result/resolve was provided; update the function resolveRunResult<R>(options?:
DryRunMessage & Partial<DryRunRunResult<R>>) to throw when options is falsy or
when neither 'resolve' nor 'result' is present, leaving the existing branches
for options.resolve() and options.result intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ffb1c7-cb1d-4abe-8095-66cdff39b80f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
package.jsonpackages/docs/src/guide/essentials/plugin-system.mdpackages/docs/src/guide/plugin/introduction.mdpackages/docs/src/guide/plugin/list.mdpackages/plugin-dryrun/README.mdpackages/plugin-dryrun/docs/functions/default.mdpackages/plugin-dryrun/docs/index.mdpackages/plugin-dryrun/docs/interfaces/DryRunExtension.mdpackages/plugin-dryrun/docs/interfaces/DryRunMessage.mdpackages/plugin-dryrun/docs/interfaces/DryRunPluginOptions.mdpackages/plugin-dryrun/docs/type-aliases/DryRunOptionsArg.mdpackages/plugin-dryrun/docs/type-aliases/DryRunRunResult.mdpackages/plugin-dryrun/docs/type-aliases/DryRunWrapResult.mdpackages/plugin-dryrun/docs/type-aliases/PluginId.mdpackages/plugin-dryrun/docs/variables/pluginId.mdpackages/plugin-dryrun/jsr.jsonpackages/plugin-dryrun/package.jsonpackages/plugin-dryrun/src/index.test-d.tspackages/plugin-dryrun/src/index.test.tspackages/plugin-dryrun/src/index.tspackages/plugin-dryrun/typedoc.config.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Linked Issues
Additional context
Summary by CodeRabbit
New Features
@gunshi/plugin-dryrunplugin providing dry-run support. Enables selective skipping of wrapped side-effect operations via--dry-runflag while allowing the rest of the command to execute normally.Documentation