Merge pull request #12372 from gitbutlerapp/inclusive-review-naming

Be oh-so-inclusive with the review terminology

Author: estib-vega

crates/but/src/args/mod.rs

@@ -531,10 +531,10 @@ pub enum Subcommands {
         id: String,
     },
 
-    /// Commands for creating and managing pull requests on a forge.
+    /// Commands for creating and managing reviews on a forge, e.g. GitHub PRs or GitLab MRs.
     ///
     /// If you are authenticated with a forge using `but config forge auth`, you can use
-    /// the `but pr` commands to create pull requests (or merge requests) on
+    /// the `but pr` or `but mr` commands to create pull requests (or merge requests) on
     /// the remote repository for your branches.
     ///
     /// Running `but pr` without a subcommand defaults to `but pr new`, which

crates/but/src/command/legacy/forge/review.rs

@@ -60,18 +60,18 @@ pub fn set_review_template(
     Ok(())
 }
 
-/// Create a new PR for a branch.
+/// Create a new forge review for a branch.
 /// If no branch is specified, prompts the user to select one.
-/// If there is only one branch without a PR, asks for confirmation.
+/// If there is only one branch without a an acco, asks for confirmation.
 #[allow(clippy::too_many_arguments)]
-pub async fn create_pr(
+pub async fn create_review(
     ctx: &mut Context,
     branch: Option<String>,
     skip_force_push_protection: bool,
     with_force: bool,
     run_hooks: bool,
     default: bool,
-    message: Option<PrMessage>,
+    message: Option<ForgeReviewMessage>,
     out: &mut OutputChannel,
 ) -> anyhow::Result<()> {
     // Fail fast if no forge user is authenticated, before pushing or prompting.
@@ -90,7 +90,7 @@ pub async fn create_pr(
 
         if branches_without_prs.is_empty() {
             if let Some(out) = out.for_human() {
-                writeln!(out, "All branches already have PRs.")?;
+                writeln!(out, "All branches already have reviews.")?;
             }
             return Ok(());
         } else if branches_without_prs.len() == 1 {
@@ -100,7 +100,7 @@ pub async fn create_pr(
                 .prepare_for_terminal_input()
                 .context("Terminal input not available. Please specify a branch using command line arguments.")?;
             if inout.confirm(
-                format!("Do you want to open a new PR on branch '{branch_name}'?"),
+                format!("Do you want to open a new review on branch '{branch_name}'?"),
                 ConfirmDefault::Yes,
             )? == Confirm::Yes
             {
@@ -149,14 +149,27 @@ async fn ensure_forge_authentication(ctx: &mut Context) -> Result<(), anyhow::Er
     let account_validity =
         but_forge::check_forge_account_is_valid(preferred_forge_user, &forge_repo_info, &storage).await?;
 
+    let forge_display_name = match forge_repo_info.forge {
+        but_forge::ForgeName::Azure => {
+            anyhow::bail!("Azure is unsupported at the minute. Sorry 😞.");
+        }
+        but_forge::ForgeName::Bitbucket => {
+            anyhow::bail!("Bitbucket is unsupported at the minute. Sorry 😞.");
+        }
+        but_forge::ForgeName::GitHub => "GitHub",
+        but_forge::ForgeName::GitLab => "GitLab",
+    };
+
     match account_validity {
         but_forge::ForgeAccountValidity::Invalid => Err(anyhow::anyhow!(
-            "Known account is not correctly authenticated.\nRun '{}' to authenticate with GitHub or GitLab.",
-            "but config forge auth"
+            "Known account is not correctly authenticated.\nRun '{}' to authenticate with {}.",
+            "but config forge auth",
+            forge_display_name
         )),
         but_forge::ForgeAccountValidity::NoCredentials => Err(anyhow::anyhow!(
-            "No authenticated forge users found.\nRun '{}' to authenticate with GitHub or GitLab.",
-            "but config forge auth"
+            "No authenticated forge users found.\nRun '{}' to authenticate with {}.",
+            "but config forge auth",
+            forge_display_name
         )),
         but_forge::ForgeAccountValidity::Valid => {
             // All good, continue
@@ -212,7 +225,7 @@ pub async fn handle_multiple_branches_in_workspace(
     with_force: bool,
     run_hooks: bool,
     default_message: bool,
-    message: Option<&PrMessage>,
+    message: Option<&ForgeReviewMessage>,
     out: &mut OutputChannel,
     selected_branches: Option<Vec<String>>,
 ) -> anyhow::Result<()> {
@@ -371,7 +384,7 @@ async fn publish_reviews_for_branch_and_dependents(
     with_force: bool,
     run_hooks: bool,
     default_message: bool,
-    message: Option<&PrMessage>,
+    message: Option<&ForgeReviewMessage>,
     out: &mut OutputChannel,
 ) -> Result<PublishReviewsOutcome, anyhow::Error> {
     let base_branch = gitbutler_branch_actions::base::get_base_branch_data(ctx)?;
@@ -543,12 +556,12 @@ enum PublishReviewResult {
 }
 
 #[derive(Clone, Debug)]
-pub struct PrMessage {
+pub struct ForgeReviewMessage {
     pub title: String,
     pub body: String,
 }
 
-pub fn parse_pr_message(content: &str) -> anyhow::Result<PrMessage> {
+pub fn parse_review_message(content: &str) -> anyhow::Result<ForgeReviewMessage> {
     let mut lines = content.lines();
     let title = lines.next().unwrap_or("").trim().to_string();
 
@@ -564,7 +577,7 @@ pub fn parse_pr_message(content: &str) -> anyhow::Result<PrMessage> {
         .trim()
         .to_string();
 
-    Ok(PrMessage { title, body })
+    Ok(ForgeReviewMessage { title, body })
 }
 
 async fn publish_review_for_branch(
@@ -574,7 +587,7 @@ async fn publish_review_for_branch(
     target_branch: &str,
     review_map: &std::collections::HashMap<String, Vec<but_forge::ForgeReview>>,
     default_message: bool,
-    message: Option<&PrMessage>,
+    message: Option<&ForgeReviewMessage>,
 ) -> anyhow::Result<PublishReviewResult> {
     // Check if a review already exists for the branch.
     // If it does, skip publishing a new review.
@@ -746,7 +759,7 @@ fn get_pr_title_and_body_from_editor(
     template.push_str("#\n");
 
     let content = get_text::from_editor_no_comments("pr_message", &template)?.to_string();
-    let message = parse_pr_message(&content)?;
+    let message = parse_review_message(&content)?;
     Ok((message.title, message.body))
 }
 
@@ -835,56 +848,56 @@ mod tests {
 
     #[test]
     fn parse_pr_message_title_only() {
-        let msg = parse_pr_message("My PR Title").unwrap();
+        let msg = parse_review_message("My PR Title").unwrap();
         assert_eq!(msg.title, "My PR Title");
         assert_eq!(msg.body, "");
     }
 
     #[test]
     fn parse_pr_message_title_and_body() {
-        let msg = parse_pr_message("My PR Title\n\nThis is the body.").unwrap();
+        let msg = parse_review_message("My PR Title\n\nThis is the body.").unwrap();
         assert_eq!(msg.title, "My PR Title");
         assert_eq!(msg.body, "This is the body.");
     }
 
     #[test]
     fn parse_pr_message_multiline_body() {
-        let msg = parse_pr_message("Title\n\nLine 1\nLine 2\nLine 3").unwrap();
+        let msg = parse_review_message("Title\n\nLine 1\nLine 2\nLine 3").unwrap();
         assert_eq!(msg.title, "Title");
         assert_eq!(msg.body, "Line 1\nLine 2\nLine 3");
     }
 
     #[test]
     fn parse_pr_message_skips_blank_lines_between_title_and_body() {
-        let msg = parse_pr_message("Title\n\n\n\nBody starts here").unwrap();
+        let msg = parse_review_message("Title\n\n\n\nBody starts here").unwrap();
         assert_eq!(msg.title, "Title");
         assert_eq!(msg.body, "Body starts here");
     }
 
     #[test]
     fn parse_pr_message_trims_whitespace() {
-        let msg = parse_pr_message("  Title with spaces  \n\n  Body with spaces  ").unwrap();
+        let msg = parse_review_message("  Title with spaces  \n\n  Body with spaces  ").unwrap();
         assert_eq!(msg.title, "Title with spaces");
         assert_eq!(msg.body, "Body with spaces");
     }
 
     #[test]
     fn parse_pr_message_empty_string_fails() {
-        let result = parse_pr_message("");
+        let result = parse_review_message("");
         assert!(result.is_err());
         assert!(result.unwrap_err().to_string().contains("empty PR title"));
     }
 
     #[test]
     fn parse_pr_message_whitespace_only_fails() {
-        let result = parse_pr_message("   \n\n   ");
+        let result = parse_review_message("   \n\n   ");
         assert!(result.is_err());
         assert!(result.unwrap_err().to_string().contains("empty PR title"));
     }
 
     #[test]
     fn parse_pr_message_blank_first_line_fails() {
-        let result = parse_pr_message("\nActual title on second line");
+        let result = parse_review_message("\nActual title on second line");
         assert!(result.is_err());
         assert!(result.unwrap_err().to_string().contains("empty PR title"));
     }

crates/but/src/lib.rs

@@ -805,52 +805,51 @@ async fn match_subcommand(
                 }) => {
                     // Read message content from file or inline
                     let message_content = match &file {
-                        Some(path) => Some(
-                            std::fs::read_to_string(path)
-                                .with_context(|| format!("Failed to read PR message from file: {}", path.display()))?,
-                        ),
+                        Some(path) => Some(std::fs::read_to_string(path).with_context(|| {
+                            format!("Failed to read forge review message from file: {}", path.display())
+                        })?),
                         None => message.clone(),
                     };
                     // Parse early to fail fast on invalid content
-                    let pr_message = match message_content {
-                        Some(content) => Some(command::legacy::forge::review::parse_pr_message(&content)?),
+                    let review_message = match message_content {
+                        Some(content) => Some(command::legacy::forge::review::parse_review_message(&content)?),
                         None => None,
                     };
                     // Check for non-interactive environment
                     if !out.can_prompt() {
                         if branch.is_none() {
                             anyhow::bail!("Non-interactive environment detected. Please specify a branch.");
                         }
-                        if pr_message.is_none() && !default {
+                        if review_message.is_none() && !default {
                             anyhow::bail!(
                                 "Non-interactive environment detected. Provide one of: --message (-m), --file (-F), or --default (-t)."
                             );
                         }
                     }
-                    command::legacy::forge::review::create_pr(
+                    command::legacy::forge::review::create_review(
                         &mut ctx,
                         branch,
                         skip_force_push_protection,
                         with_force,
                         run_hooks,
                         default,
-                        pr_message,
+                        review_message,
                         out,
                     )
                     .await
-                    .context("Failed to create PR for branch.")
+                    .context("Failed to create forge review for branch.")
                     .emit_metrics(metrics_ctx)
                 }
                 Some(forge::pr::Subcommands::Template { template_path }) => {
                     command::legacy::forge::review::set_review_template(&mut ctx, template_path, out)
-                        .context("Failed to set PR template.")
+                        .context("Failed to set forge review template.")
                         .emit_metrics(metrics_ctx)
                 }
                 None => {
                     // Default to `pr new` when no subcommand is provided
-                    command::legacy::forge::review::create_pr(&mut ctx, None, false, true, true, false, None, out)
+                    command::legacy::forge::review::create_review(&mut ctx, None, false, true, true, false, None, out)
                         .await
-                        .context("Failed to create PR for branch.")
+                        .context("Failed to create forge review for branch.")
                         .emit_metrics(metrics_ctx)
                 }
             }