From fc89d3a63dbeccba2beb4e14a689c3021f63b421 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Thu, 21 May 2026 11:22:41 -0700 Subject: [PATCH] refactor(cli): consolidate --no-verify deprecation into one path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deprecated `--no-verify` flag was declared as a clap arg five times across SwitchArgs, RemoveArgs, CommitArgs, SquashArgs, and MergeArgs under two field names, resolved through two helpers, with the deprecation warning string hand-duplicated in `resolve_verify` and `handle_merge_command`. Collapse the four bool-resolving commands (switch, remove, step commit, step squash) onto a single flattened `HookFlags` args struct with one `resolve()` method, and route every `--no-verify` deprecation warning — including merge's — through one `warn_no_verify_deprecated` emitter, so the warning text exists in exactly one place. `wt merge` keeps its own three flags: its hooks flag is tri-state (`Option`, so config `[merge] verify` can still apply) and it carries a positive `--verify` override, which `HookFlags`'s `SetFalse`/default-true shape cannot express. It now shares only the warning emitter. Behavior is unchanged — `--no-verify` still works as a deprecated alias and emits the same warning. The one visible `--help` change: `--no-hooks` for `step commit`/`step squash` moves under the "Automation" heading, matching where switch/remove/merge already place it (`HookFlags` carries the heading); generated step.md mirrors regenerated to match. Adds a `remove --no-verify` deprecation test alongside the existing switch and merge coverage. Co-Authored-By: Claude Opus 4.7 --- docs/content/step.md | 12 +++---- skills/worktrunk/reference/step.md | 12 +++---- src/cli/mod.rs | 51 ++++++++++++++++++++++-------- src/cli/step.rs | 18 +++-------- src/main.rs | 35 +++++++++----------- tests/integration_tests/remove.rs | 33 +++++++++++++++++++ 6 files changed, 101 insertions(+), 60 deletions(-) diff --git a/docs/content/step.md b/docs/content/step.md index e7567dee09..20063c8c2c 100644 --- a/docs/content/step.md +++ b/docs/content/step.md @@ -138,9 +138,6 @@ Usage: wt step commit [OPTIONS] -b, --branch <BRANCH> Branch to operate on (defaults to current worktree) - --no-hooks - Skip hooks - --stage <STAGE> What to stage before committing [default: all] @@ -156,6 +153,9 @@ Usage: wt step commit [OPTIONS] Print help (see a summary with '-h') Automation: + --no-hooks + Skip hooks + --format <FORMAT> Output format @@ -233,9 +233,6 @@ Usage: wt step squash [OPTIONS] Defaults to default branch. Options: - --no-hooks - Skip hooks - --stage <STAGE> What to stage before committing [default: all] @@ -251,6 +248,9 @@ Usage: wt step squash [OPTIONS] Print help (see a summary with '-h') Automation: + --no-hooks + Skip hooks + --format <FORMAT> Output format diff --git a/skills/worktrunk/reference/step.md b/skills/worktrunk/reference/step.md index d945e5a830..bd9680f2cc 100644 --- a/skills/worktrunk/reference/step.md +++ b/skills/worktrunk/reference/step.md @@ -133,9 +133,6 @@ Options: -b, --branch Branch to operate on (defaults to current worktree) - --no-hooks - Skip hooks - --stage What to stage before committing [default: all] @@ -151,6 +148,9 @@ Options: Print help (see a summary with '-h') Automation: + --no-hooks + Skip hooks + --format Output format @@ -232,9 +232,6 @@ Arguments: Defaults to default branch. Options: - --no-hooks - Skip hooks - --stage What to stage before committing [default: all] @@ -250,6 +247,9 @@ Options: Print help (see a summary with '-h') Automation: + --no-hooks + Skip hooks + --format Output format diff --git a/src/cli/mod.rs b/src/cli/mod.rs index e26a114220..c4f96ef767 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -266,6 +266,39 @@ pub(crate) struct Cli { pub command: Option, } +/// Shared `--no-hooks` / `--no-verify` flags for commands that resolve hook +/// skipping to a plain `bool` (`switch`, `remove`, `step commit`, +/// `step squash`). +/// +/// `wt merge` does not flatten this struct: its hooks flag is tri-state +/// (`Option`, so config `[merge] verify` can still apply) and it carries +/// a positive `--verify` override. It declares its own flags but routes the +/// `--no-verify` deprecation through `crate::warn_no_verify_deprecated`, so the +/// warning text lives in exactly one place. +#[derive(Args)] +pub(crate) struct HookFlags { + /// Skip hooks + #[arg(long = "no-hooks", action = clap::ArgAction::SetFalse, default_value_t = true, help_heading = "Automation")] + pub(crate) verify: bool, + + /// Skip hooks (deprecated alias for --no-hooks) + #[arg(long = "no-verify", hide = true)] + pub(crate) no_verify_deprecated: bool, +} + +impl HookFlags { + /// Resolve to the effective verify value, emitting the deprecation warning + /// once if `--no-verify` was used. + pub(crate) fn resolve(&self) -> bool { + if self.no_verify_deprecated { + crate::warn_no_verify_deprecated(); + false + } else { + self.verify + } + } +} + #[derive(Args)] pub(crate) struct SwitchArgs { /// Branch name or shortcut @@ -344,13 +377,8 @@ pub(crate) struct SwitchArgs { #[arg(long, overrides_with = "no_cd", hide = true)] pub(crate) cd: bool, - /// Skip hooks - #[arg(long = "no-hooks", action = clap::ArgAction::SetFalse, default_value_t = true, help_heading = "Automation")] - pub(crate) verify: bool, - - /// Skip hooks (deprecated alias for --no-hooks) - #[arg(long = "no-verify", hide = true)] - pub(crate) no_verify_deprecated: bool, + #[command(flatten)] + pub(crate) hooks: HookFlags, /// Output format /// @@ -420,13 +448,8 @@ pub(crate) struct RemoveArgs { #[arg(long)] pub(crate) foreground: bool, - /// Skip hooks - #[arg(long = "no-hooks", action = clap::ArgAction::SetFalse, default_value_t = true, help_heading = "Automation")] - pub(crate) verify: bool, - - /// Skip hooks (deprecated alias for --no-hooks) - #[arg(long = "no-verify", hide = true)] - pub(crate) no_verify_deprecated: bool, + #[command(flatten)] + pub(crate) hooks: HookFlags, /// Force worktree removal /// diff --git a/src/cli/step.rs b/src/cli/step.rs index ff64926acc..0f0f40b580 100644 --- a/src/cli/step.rs +++ b/src/cli/step.rs @@ -6,13 +6,8 @@ pub struct CommitArgs { #[arg(short, long, add = crate::completion::worktree_only_completer())] pub(crate) branch: Option, - /// Skip hooks - #[arg(long = "no-hooks", action = clap::ArgAction::SetFalse, default_value_t = true)] - pub(crate) verify: bool, - - /// Skip hooks (deprecated alias for --no-hooks) - #[arg(long = "no-verify", hide = true)] - pub(crate) no_verify_deprecated: bool, + #[command(flatten)] + pub(crate) hooks: crate::cli::HookFlags, /// What to stage before committing [default: all] #[arg(long)] @@ -41,13 +36,8 @@ pub struct SquashArgs { #[arg(add = crate::completion::branch_value_completer())] pub(crate) target: Option, - /// Skip hooks - #[arg(long = "no-hooks", action = clap::ArgAction::SetFalse, default_value_t = true)] - pub(crate) verify: bool, - - /// Skip hooks (deprecated alias for --no-hooks) - #[arg(long = "no-verify", hide = true)] - pub(crate) no_verify_deprecated: bool, + #[command(flatten)] + pub(crate) hooks: crate::cli::HookFlags, /// What to stage before committing [default: all] #[arg(long)] diff --git a/src/main.rs b/src/main.rs index 9425fd3f04..6262a2156f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -144,18 +144,16 @@ fn warn_select_deprecated() { ); } -/// Resolve the `--no-hooks` / `--no-verify` pair: emit a deprecation warning -/// if the old flag was used, then return the effective verify value. -fn resolve_verify(verify: bool, no_verify_deprecated: bool) -> bool { - if no_verify_deprecated { - eprintln!( - "{}", - warning_message("--no-verify is deprecated; use --no-hooks instead") - ); - false - } else { - verify - } +/// Emit the canonical `--no-verify` deprecation warning to stderr. +/// +/// Single source of this warning text. `HookFlags::resolve` (switch, remove, +/// step commit, step squash) and `handle_merge_command` both call here, so the +/// message stays identical across every command that accepts `--no-verify`. +pub(crate) fn warn_no_verify_deprecated() { + eprintln!( + "{}", + warning_message("--no-verify is deprecated; use --no-hooks instead") + ); } fn handle_hook_command(action: HookCommand, yes: bool) -> anyhow::Result<()> { @@ -201,7 +199,7 @@ fn handle_hook_command(action: HookCommand, yes: bool) -> anyhow::Result<()> { fn handle_step_command(action: StepCommand, yes: bool) -> anyhow::Result<()> { match action { StepCommand::Commit(args) => { - let verify = resolve_verify(args.verify, args.no_verify_deprecated); + let verify = args.hooks.resolve(); let format = args.format; // `--show-prompt` and `--dry-run` emit raw text (rendered prompt or LLM // preview), which would corrupt a JSON consumer's stdout. Refuse the @@ -230,7 +228,7 @@ fn handle_step_command(action: StepCommand, yes: bool) -> anyhow::Result<()> { Ok(()) } StepCommand::Squash(args) => { - let verify = resolve_verify(args.verify, args.no_verify_deprecated); + let verify = args.hooks.resolve(); // `--show-prompt` and `--dry-run` emit raw text (rendered prompt or LLM // preview), which would corrupt a JSON consumer's stdout. if args.format == SwitchFormat::Json && (args.show_prompt || args.dry_run) { @@ -665,7 +663,7 @@ fn handle_select_command(_branches: bool, _remotes: bool) -> anyhow::Result<()> } fn handle_switch_command(args: SwitchArgs, yes: bool) -> anyhow::Result<()> { - let verify = resolve_verify(args.verify, args.no_verify_deprecated); + let verify = args.hooks.resolve(); // With no branch argument, `wt switch` opens a TUI picker — config // deprecation warnings would render above the picker and push it down. @@ -891,7 +889,7 @@ fn validate_remove_targets( /// or success message. See [`commands::process::run_internal_sweep`]. fn handle_remove_command(args: RemoveArgs, yes: bool) -> anyhow::Result<()> { let json_mode = args.format == SwitchFormat::Json; - let verify = resolve_verify(args.verify, args.no_verify_deprecated); + let verify = args.hooks.resolve(); UserConfig::load() .context("Failed to load config") .and_then(|config| { @@ -1326,10 +1324,7 @@ fn init_logging(verbose_level: u8) { fn handle_merge_command(args: MergeArgs, yes: bool) -> anyhow::Result<()> { if args.no_verify { - eprintln!( - "{}", - warning_message("--no-verify is deprecated; use --no-hooks instead") - ); + warn_no_verify_deprecated(); } handle_merge(MergeOptions { target: args.target.as_deref(), diff --git a/tests/integration_tests/remove.rs b/tests/integration_tests/remove.rs index 329b24d9d4..6f1bdacd03 100644 --- a/tests/integration_tests/remove.rs +++ b/tests/integration_tests/remove.rs @@ -2107,6 +2107,39 @@ approved-commands = ["echo 'hook ran' > {}"] ); } +#[rstest] +fn test_remove_no_verify_deprecated_still_works(mut repo: TestRepo) { + let worktree_path = repo.add_worktree("feature-no-verify"); + + // --no-verify is a deprecated alias for --no-hooks: still works, still + // emits the shared deprecation warning that points at --no-hooks. + let output = repo + .wt_command() + .args([ + "remove", + "--foreground", + "--yes", + "--no-verify", + "feature-no-verify", + ]) + .output() + .unwrap(); + assert!(output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("--no-verify is deprecated"), + "Expected deprecation warning in stderr: {stderr}" + ); + assert!( + stderr.contains("--no-hooks"), + "Expected --no-hooks suggestion in stderr: {stderr}" + ); + assert!( + !worktree_path.exists(), + "Worktree should be removed with --no-verify" + ); +} + /// /// Even when a worktree is in detached HEAD state (no branch), the pre-remove /// hook should still execute.