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() {