refactor(cli): replace sandbox_create positional args with SandboxCreateConfig struct#1997
refactor(cli): replace sandbox_create positional args with SandboxCreateConfig struct#1997lunarwhite wants to merge 1 commit into
Conversation
…ateConfig struct Signed-off-by: Yuedong Wu <dwcn22@outlook.com>
|
/ok-to-test 83637ad |
| let tls = test_tls(&server); | ||
| install_fake_ssh(&fake_ssh_dir); | ||
|
|
||
| let cmd = vec!["echo".into(), "OK".into()]; |
There was a problem hiding this comment.
Not that it's too important, but can one specify this inline?
There was a problem hiding this comment.
I don't have a preference. Looking at the broader codebase style, not-inline seems to be the more adopted pattern. If you don't have a strong preference on this either, I'll leave it as-is for now. 🙂
There was a problem hiding this comment.
Actually, looking at this again. I think specifying this inline if possible will be preferred as it aligns better with how all other options are handled.
elezar
left a comment
There was a problem hiding this comment.
LGTM!
Are there other places where a similar cleanup (as follow-ups) would make sense?
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. |
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