refactor(cli): replace sandbox_create positional args with SandboxCreateConfig struct#1997
Merged
Merged
Conversation
elezar
reviewed
Jun 25, 2026
Member
|
/ok-to-test 83637ad |
elezar
reviewed
Jun 25, 2026
elezar
previously approved these changes
Jun 25, 2026
elezar
left a comment
Member
There was a problem hiding this comment.
LGTM!
Are there other places where a similar cleanup (as follow-ups) would make sense?
Contributor
Author
Good question @elezar. I just tried to create a spike with help of AI agent: #2003. But its scope looks too broad. Most remaining items have 8 params (barely above clippy's default threshold of 7) and 1-2 call sites. Only two functions approach #1408's severity:
IMHO neither individually justifies the same urgency this PR. Please feel free to modify or close it in favor of your deep review. |
…ateConfig struct Signed-off-by: Yuedong Wu <dwcn22@outlook.com>
83637ad to
507940d
Compare
Member
|
/ok-to-test 507940d |
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
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.
Summary
Replace the 21-parameter positional signature of
run::sandbox_createwith aSandboxCreateConfigstruct, following theProviderRefreshConfigInputprecedent established in PR #1349. This removes theclippy::too_many_argumentssuppression and makes call sites self-documenting via named fields and struct update syntax.Related Issue
Closes #1408
Notes for reviewers:
The original issue suggested a builder pattern (
SandboxCreateBuilder::new(...).name(...).create().await?).This PR proposes to use a plain config struct with
Defaultinstead, for two reasons:ProviderRefreshConfigInput(PR feat(providers): add credential refresh foundation #1349, merged after the issue was filed) established a config-struct convention in the CLI crate, while no builder pattern exists incrates/openshell-cli/.sandbox_createis an async side-effecting operation, not an inert value construction. A builder would require ~18 setter methods for the same named-field ergonomics thatSandboxCreateConfig { ..Default::default() }provides for free. The struct approach solves the stated problem (positional args, readability, fragility) whilestaying consistent with the codebase.
Changes
SandboxCreateConfig<'a>struct withDefaultimpl (safe production defaults:keep: false,approval_mode: "manual")sandbox_create()to accept(server, gateway_name, config, tls), keeping infrastructure params positional per existing conventionmain.rsto construct the config structtest_config()helper in integration tests with test-appropriate defaults (keep: true,tty_override: Some(false)) and migrate all 14 call sitesTesting
mise run pre-commitpassesChecklist