From 84203a3f9b88580e6231f05dc12baeab18e84c2f Mon Sep 17 00:00:00 2001 From: atlas Date: Fri, 11 Sep 2026 03:18:44 +0200 Subject: [PATCH] hive-forge: let pr status take the PR number positionally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other PR-scoped verb takes the number as a positional — `pr show 42`, `pr comments 42`, `pr assign-reviewer 42 argus`. `pr status` alone required `--pr 42`, so whichever form you learn first is wrong for the other, and clap's error for the mistake suggests `-- --pr`, which would pass the literal string on as the next positional. The number is positional here too now. `--pr` stays, because it was the only spelling this verb had; `--sha` keeps its flag because the two are alternatives rather than one required argument. Two error strings in this file also named `pr-status`, a form that has refused to run since the subcommand rename. One of them is the message you get for passing neither argument — i.e. exactly when you are already unsure what the verb is called. The four new cases cover both halves: that clap accepts each spelling, and that something reads it. `target_pr` is named rather than inlined for exactly that reason — dropping the positional from the selection passed every parse-only case while leaving `pr status 42` reporting "pass a PR number". The conflict case earns its keep the same way: clap accepting an argument and clap ignoring it are indistinguishable from a passing parse, so `42 --sha abc` and `42 --pr 42` both have to be rejected. Closes #4182. --- hive-forge/src/verbs/pr_status.rs | 89 +++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 10 deletions(-) diff --git a/hive-forge/src/verbs/pr_status.rs b/hive-forge/src/verbs/pr_status.rs index 246517d3..3802f6cf 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -1,12 +1,12 @@ -//! `pr status --pr ` — one-stop PR health view: mergeable state, CI +//! `pr status ` — one-stop PR health view: mergeable state, CI //! checks, requested reviewers + review verdicts, and the last-comment //! timestamp. `--sha ` is a CI-only fast path for a raw commit. //! Removes the need for raw `curl` to the statuses / reviews endpoints, //! keeping forge access on the single `hive-forge` tool. //! -//! Exit code is a merge-readiness verdict for `--pr`: 0 only when CI is -//! green AND the PR is mergeable AND no review requests changes — so it -//! composes (`hive-forge pr status --pr 42 && …`). `--sha` mirrors the +//! Exit code is a merge-readiness verdict when given a PR: 0 only when +//! CI is green AND the PR is mergeable AND no review requests changes — +//! so it composes (`hive-forge pr status 42 && …`). `--sha` mirrors the //! CI verdict alone (0 = success). use anyhow::{Context, Result, bail}; @@ -21,26 +21,42 @@ use crate::verbs::{print_json, rfc3339}; pub struct Args { /// PR number — full health view (mergeable, CI, reviews, last /// comment). Mutually exclusive with `--sha`. + #[arg(value_name = "NUMBER", conflicts_with_all = ["pr", "sha"])] + number: Option, + /// Same as passing the number positionally. Kept because this verb + /// accepted only this spelling before the positional existed. #[arg(long, conflicts_with = "sha")] pr: Option, /// Explicit commit sha (or ref) — CI-only fast path. Mutually - /// exclusive with `--pr`. + /// exclusive with a PR number. #[arg(long)] sha: Option, } +impl Args { + /// The PR to inspect, whichever spelling asked for it. + /// + /// Named rather than inlined so the two spellings can be shown to + /// select the same PR: a test that only parses proves clap accepted + /// the positional, never that anything reads it. + fn target_pr(&self) -> Option { + self.number.or(self.pr) + } +} + /// # Errors /// -/// Returns an error if neither `--pr` nor `--sha` is given, or any forge -/// GET fails (PR lookup, combined status, reviews, comments). A +/// Returns an error if neither a PR number nor `--sha` is given, or any +/// forge GET fails (PR lookup, combined status, reviews, comments). A /// not-ready verdict is NOT an error — it's reported and reflected in /// the process exit code instead. pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo()?; - match (args.pr, args.sha) { + let target = args.target_pr(); + match (target, args.sha) { (Some(pr), _) => pr_status(client, repo, pr), (None, Some(sha)) => sha_status(client, &sha), - (None, None) => bail!("pr-status: pass one of --pr or --sha "), + (None, None) => bail!("pr status: pass a PR number (`pr status 42`) or --sha "), } } @@ -104,7 +120,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { .head .as_ref() .and_then(|h| h.sha.clone()) - .with_context(|| format!("pr-status: PR #{pr} has no head.sha"))?; + .with_context(|| format!("pr status: PR #{pr} has no head.sha"))?; let requested: Vec = pull .requested_reviewers @@ -389,6 +405,59 @@ fn combined_json(sha: &str, state: &str, statuses: &[Value]) -> Value { #[cfg(test)] mod tests { use super::*; + use clap::Parser; + + /// `Args` is a flattened arg group, not a `Parser`, so parsing it + /// standalone needs a wrapper. Without one these cases could only be + /// written against the whole binary's clap tree. + #[derive(Parser)] + struct Wrap { + #[command(flatten)] + args: Args, + } + + fn parse(argv: &[&str]) -> Result { + Wrap::try_parse_from(argv).map(|w| w.args) + } + + /// The property: the number is accepted the way every other + /// `pr ` takes it, and the older flag still works. + #[test] + fn the_pr_number_is_accepted_positionally_and_as_a_flag() { + assert_eq!(parse(&["x", "42"]).unwrap().number, Some(42)); + assert_eq!(parse(&["x", "--pr", "42"]).unwrap().pr, Some(42)); + } + + /// And something reads it. Parsing only proves clap accepted the + /// positional — dropping it from the selection leaves every other + /// case here green while `pr status 42` reports "pass a PR number". + #[test] + fn both_spellings_select_the_same_pr_and_sha_selects_none() { + assert_eq!(parse(&["x", "42"]).unwrap().target_pr(), Some(42)); + assert_eq!(parse(&["x", "--pr", "42"]).unwrap().target_pr(), Some(42)); + assert_eq!(parse(&["x", "--sha", "abc"]).unwrap().target_pr(), None); + } + + /// Control for the case above. Clap accepting an argument and clap + /// ignoring it look identical from a passing parse, so the conflicts + /// have to be shown to bite. + #[test] + fn a_pr_number_conflicts_with_sha_and_with_the_flag() { + assert!(parse(&["x", "42", "--sha", "abc"]).is_err()); + assert!(parse(&["x", "42", "--pr", "42"]).is_err()); + } + + /// Giving neither is a runtime error from `run`, not a parse error — + /// so `--sha` alone has to keep parsing. + #[test] + fn neither_form_parses_and_sha_alone_still_does() { + let empty = parse(&["x"]).unwrap(); + assert!(empty.number.is_none() && empty.pr.is_none() && empty.sha.is_none()); + assert_eq!( + parse(&["x", "--sha", "abc"]).unwrap().sha.as_deref(), + Some("abc") + ); + } #[test] fn short_sha_truncates() {