diff --git a/CHANGELOG.md b/CHANGELOG.md index 166838f0..4ce1e377 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 merged. The Git remote helper will now fallback to the canonical default branch, if the default branch of the delegate could not be found, and correctly mark the patch as merged. +- `rad patch review` presents an appropriate error to the user when the user + does not provide an option, one of `--accept` or `--reject`, or a summary + message. ## 1.9.1 diff --git a/crates/radicle-cli/examples/rad-patch-review-no-options.md b/crates/radicle-cli/examples/rad-patch-review-no-options.md new file mode 100644 index 00000000..4853ec64 --- /dev/null +++ b/crates/radicle-cli/examples/rad-patch-review-no-options.md @@ -0,0 +1,24 @@ +The `rad patch review` command must provide an option or a review message, since +a review must have a verdict or a review summary: + +``` (fail) +$ rad patch review aa45913 --no-message --no-announce +✗ Error: expected one of `--accept` or `--reject`, or supply a review message +``` + +This time we will supply a message: + +``` +$ rad patch review aa45913 -m "Looks reasonable, no strong opinion." --no-announce +✓ Patch aa45913 reviewed +``` + +Submitting a second review on the same revision by the same author is ignored. +Currently, there is no way to edit a review from the CLI. + +Note that the review command will still succeed, but with no effect: + +``` +$ rad patch review aa45913 -m "Changed my mind." --no-announce +✓ Patch aa45913 reviewed +``` diff --git a/crates/radicle-cli/src/commands/patch.rs b/crates/radicle-cli/src/commands/patch.rs index 23d23f0f..c3696854 100644 --- a/crates/radicle-cli/src/commands/patch.rs +++ b/crates/radicle-cli/src/commands/patch.rs @@ -17,6 +17,7 @@ mod review; mod show; mod update; +use core::convert::TryInto; use std::collections::BTreeSet; use anyhow::anyhow; @@ -196,7 +197,13 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { .map(|rev| rev.resolve::(&repository.backend)) .transpose()? .map(patch::RevisionId::from); - review::run(patch_id, revision_id, options.into(), &profile, &repository)?; + review::run( + patch_id, + revision_id, + options.try_into()?, + &profile, + &repository, + )?; } Command::Resolve { diff --git a/crates/radicle-cli/src/commands/patch/args.rs b/crates/radicle-cli/src/commands/patch/args.rs index b6de2dcd..0db3a6e7 100644 --- a/crates/radicle-cli/src/commands/patch/args.rs +++ b/crates/radicle-cli/src/commands/patch/args.rs @@ -1,3 +1,5 @@ +use core::result::Result::Err; + use clap::{Parser, Subcommand}; use radicle::cob::Label; @@ -588,8 +590,15 @@ pub(super) struct ReviewArgs { message_args: MessageArgs, } +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum OperationError { + #[error("expected one of `--accept` or `--reject`, or supply a review message")] + MissingOption, +} + impl ReviewArgs { - fn as_operation(&self) -> review::Operation { + fn as_operation(&self, message: &Message) -> Result { let Self { patch, accept, @@ -606,51 +615,60 @@ impl ReviewArgs { } else { None }; - return review::Operation::Review(review::ReviewOptions { + return Ok(review::Operation::Review(review::ReviewOptions { by_hunk: true, unified: self.unified, hunk: self.hunk, verdict, - }); + })); } if *delete { - return review::Operation::Delete; + return Ok(review::Operation::Delete); } if *accept { - return review::Operation::Review(review::ReviewOptions { + return Ok(review::Operation::Review(review::ReviewOptions { by_hunk: false, unified: 3, hunk: None, verdict: Some(Verdict::Accept), - }); + })); } if *reject { - return review::Operation::Review(review::ReviewOptions { + return Ok(review::Operation::Review(review::ReviewOptions { by_hunk: false, unified: 3, hunk: None, verdict: Some(Verdict::Reject), - }); + })); } - panic!("expected one of `--patch`, `--delete`, `--accept`, or `--reject`"); + if matches!(message, Message::Edit | Message::Text(_)) { + return Ok(review::Operation::Review(review::ReviewOptions { + by_hunk: false, + unified: self.unified, + hunk: self.hunk, + verdict: None, + })); + } + + Err(OperationError::MissingOption) } } -impl From for review::Options { - fn from(args: ReviewArgs) -> Self { - let op = args.as_operation(); - Self { - message: Message::from(args.message_args), - op, - } +impl TryFrom for review::Options { + type Error = OperationError; + + fn try_from(args: ReviewArgs) -> Result { + let message = Message::from(args.message_args.clone()); + let op = args.as_operation(&message)?; + Ok(Self { message, op }) } } -#[derive(Debug, clap::Args)] +#[derive(Clone, Debug, clap::Args)] #[group(required = false, multiple = false)] pub(super) struct MessageArgs { /// Provide a message (default: prompt) diff --git a/crates/radicle-cli/tests/commands/patch.rs b/crates/radicle-cli/tests/commands/patch.rs index 276d3dc7..f6125aa1 100644 --- a/crates/radicle-cli/tests/commands/patch.rs +++ b/crates/radicle-cli/tests/commands/patch.rs @@ -27,6 +27,11 @@ fn rad_patch_checkout() { Environment::alice(["rad-init", "rad-patch-checkout"]); } +#[test] +fn rad_patch_review_no_options() { + Environment::alice(["rad-init", "rad-patch", "rad-patch-review-no-options"]); +} + #[test] fn rad_patch_checkout_revision() { Environment::alice([