From d0f0913436a77d583f62765b2253cf6f07d36839 Mon Sep 17 00:00:00 2001 From: Mubashwer Salman Khurshid Date: Thu, 17 Jul 2025 02:28:20 +1000 Subject: [PATCH] feat: improve error handling with contextual error messages - Migrate from Box to anyhow for better error handling - Add contextual error messages throughout the codebase using anyhow::Context - Use bail! macro for cleaner error creation - Provide more helpful error messages for git command failures - Add specific context for team member and mob session operations - Update test expectations to match new error message format Users now get more informative error messages with context about what operation failed and why. --- Cargo.lock | 7 +++++ Cargo.toml | 1 + src/commands/mob.rs | 7 +++-- src/commands/setup.rs | 10 ++++--- src/commands/team_member.rs | 3 ++- src/helpers.rs | 5 +++- src/lib.rs | 2 +- src/repositories/mob_session_repo.rs | 17 +++++++----- src/repositories/team_member_repo.rs | 40 ++++++++++++++++++---------- tests/mob.rs | 4 +-- tests/setup.rs | 4 +-- tests/team_member.rs | 4 +-- 12 files changed, 67 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ee32451..a6b1fd7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -61,6 +61,12 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "anyhow" +version = "1.0.98" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e16d2d3311acee920a9eb8d33b8cbc1787ce4a264e85f964c2404b969bdcd487" + [[package]] name = "assert_cmd" version = "2.0.17" @@ -379,6 +385,7 @@ dependencies = [ name = "git-mob-tool" version = "1.9.2" dependencies = [ + "anyhow", "assert_cmd", "clap", "inquire", diff --git a/Cargo.toml b/Cargo.toml index 2612f7a..12c07b9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,7 @@ name = "git-mob" path = "src/main.rs" [dependencies] +anyhow = "1.0" clap = { version = "4.5.41", features = ["derive"] } inquire = "0.7.5" path-clean = "1.0.1" diff --git a/src/commands/mob.rs b/src/commands/mob.rs index e3a7d18..5fbf31f 100644 --- a/src/commands/mob.rs +++ b/src/commands/mob.rs @@ -1,5 +1,6 @@ use crate::Result; use crate::repositories::{MobSessionRepo, TeamMemberRepo}; +use anyhow::bail; use clap::{Parser, arg}; use inquire::MultiSelect; use std::io::Write; @@ -72,9 +73,7 @@ impl Mob { Some([]) => { let team_members = team_member_repo.list(false)?; if team_members.is_empty() { - return Err( - "No team member(s) found. At least one team member must be added".into(), - ); + bail!("No team member(s) found. At least one team member must be added"); } let result = MultiSelect::new("Select active co-author(s):", team_members) @@ -100,7 +99,7 @@ impl Mob { mob_repo.add_coauthor(&team_member)?; coauthors.push(team_member); } - None => return Err(format!("No team member found with key: {key}").into()), + None => bail!("No team member found with key: {key}"), } } diff --git a/src/commands/setup.rs b/src/commands/setup.rs index 9f71ea4..784d7ca 100644 --- a/src/commands/setup.rs +++ b/src/commands/setup.rs @@ -1,4 +1,5 @@ use crate::Result; +use anyhow::{anyhow, bail}; use clap::Parser; use path_clean::PathClean; use std::{ @@ -41,7 +42,7 @@ impl Setup { Some(hooks_dir) => hooks_dir, None => { let new_hooks_dir = env::home_dir() - .ok_or("Failed to get home directory")? + .ok_or_else(|| anyhow!("Failed to get home directory"))? .join(".git") .join("hooks") .clean(); @@ -73,7 +74,7 @@ impl Setup { fn handle_local(&self, out: &mut impl Write) -> Result<()> { let hooks_dir = match Self::get_hooks_dir("--local")? { Some(hooks_dir) => hooks_dir, - None => return Err("Local githooks directory is not set".into()), + None => bail!("Local githooks directory is not set"), }; let prepare_commit_msg_path = hooks_dir.join("prepare-commit-msg").clean(); @@ -108,7 +109,8 @@ impl Setup { return Ok(Some(hooks_dir)); } - let mut expanded_hooks_dir = env::home_dir().ok_or("Failed to get home directory")?; + let mut expanded_hooks_dir = + env::home_dir().ok_or_else(|| anyhow!("Failed to get home directory"))?; expanded_hooks_dir.extend(hooks_dir.components().skip(1)); Ok(Some(expanded_hooks_dir.clean())) } @@ -120,7 +122,7 @@ impl Setup { .status()?; if !status.success() { - return Err(format!("Failed to set global githooks directory to {path_str}").into()); + bail!("Failed to set global githooks directory to {path_str}"); } writeln!(out, "Set global githooks directory: {path_str}")?; diff --git a/src/commands/team_member.rs b/src/commands/team_member.rs index 4f3b24d..abebae7 100644 --- a/src/commands/team_member.rs +++ b/src/commands/team_member.rs @@ -1,5 +1,6 @@ use crate::Result; use crate::repositories::TeamMemberRepo; +use anyhow::bail; use clap::{Parser, arg}; use std::io::Write; @@ -32,7 +33,7 @@ impl TeamMember { if let Some(key) = self.delete.as_deref() { match team_member_repo.get(key)? { Some(_) => team_member_repo.remove(key)?, - None => return Err(format!("No team member found with key: {key}").into()), + None => bail!("No team member found with key: {key}"), } } if self.list { diff --git a/src/helpers.rs b/src/helpers.rs index 960bbe0..2b377c2 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -1,4 +1,5 @@ use crate::Result; +use anyhow::Context; use std::process::Command; pub struct CmdOutput { @@ -19,7 +20,9 @@ pub struct StdCommandRunner; impl CommandRunner for StdCommandRunner { fn execute(&self, program: &str, args: &[&str]) -> Result { - let output = Command::new(program).args(args).output()?; + let output = Command::new(program).args(args).output().with_context(|| { + format!("Failed to execute command: {} {}", program, args.join(" ")) + })?; Ok(CmdOutput { stdout: output.stdout, diff --git a/src/lib.rs b/src/lib.rs index 609a0bb..2f26855 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,4 @@ -pub type Result = std::result::Result>; +pub type Result = anyhow::Result; pub mod cli; mod commands; diff --git a/src/repositories/mob_session_repo.rs b/src/repositories/mob_session_repo.rs index f41d525..7afd664 100644 --- a/src/repositories/mob_session_repo.rs +++ b/src/repositories/mob_session_repo.rs @@ -1,5 +1,6 @@ use crate::Result; use crate::helpers::{CmdOutput, CommandRunner}; +use anyhow::{Context, bail}; #[cfg(test)] use mockall::{automock, predicate::*}; @@ -23,8 +24,8 @@ impl GitConfigMobRepo { fn git_config_error(output: &CmdOutput) -> Result { match output.status_code { - Some(code) => Err(format!("Git config command exited with status code: {code}").into()), - None => Err("Git config command terminated by signal".into()), + Some(code) => bail!("Git config command exited with status code: {code}"), + None => bail!("Git config command terminated by signal"), } } } @@ -35,10 +36,12 @@ impl MobSessionRepo for GitConfigMobRepo { let output = self .command_runner - .execute("git", &["config", "--global", "--get-all", &full_key])?; + .execute("git", &["config", "--global", "--get-all", &full_key]) + .context("Failed to list coauthors from git config")?; match output.status_code { - Some(Self::EXIT_CODE_SUCCESS) => Ok(String::from_utf8(output.stdout)? + Some(Self::EXIT_CODE_SUCCESS) => Ok(String::from_utf8(output.stdout) + .context("Failed to parse git config output as UTF-8")? .lines() .map(|x| x.into()) .collect()), @@ -52,7 +55,8 @@ impl MobSessionRepo for GitConfigMobRepo { let output = self .command_runner - .execute("git", &["config", "--global", "--add", &full_key, coauthor])?; + .execute("git", &["config", "--global", "--add", &full_key, coauthor]) + .with_context(|| format!("Failed to add coauthor '{coauthor}' to git config"))?; match output.status_code { Some(Self::EXIT_CODE_SUCCESS) => Ok(()), @@ -68,7 +72,8 @@ impl MobSessionRepo for GitConfigMobRepo { let output = self .command_runner - .execute("git", &["config", "--global", "--remove-section", §ion])?; + .execute("git", &["config", "--global", "--remove-section", §ion]) + .context("Failed to clear mob session from git config")?; match output.status_code { Some(Self::EXIT_CODE_SUCCESS) => Ok(()), diff --git a/src/repositories/team_member_repo.rs b/src/repositories/team_member_repo.rs index 15780bb..a7c7d44 100644 --- a/src/repositories/team_member_repo.rs +++ b/src/repositories/team_member_repo.rs @@ -1,5 +1,6 @@ use crate::Result; use crate::helpers::{CmdOutput, CommandRunner}; +use anyhow::{Context, anyhow, bail}; #[cfg(test)] use mockall::{automock, predicate::*}; @@ -24,8 +25,8 @@ impl GitConfigTeamMemberRepo { fn git_config_error(output: &CmdOutput) -> Result { match output.status_code { - Some(code) => Err(format!("Git config command exited with status code: {code}").into()), - None => Err("Git config command terminated by signal".into()), + Some(code) => bail!("Git config command exited with status code: {code}"), + None => bail!("Git config command terminated by signal"), } } } @@ -35,10 +36,13 @@ impl TeamMemberRepo for GitConfigTeamMemberRepo { let section = Self::COAUTHORS_SECTION; let search_regex = format!("^{section}\\."); - let output = self.command_runner.execute( - "git", - &["config", "--global", "--get-regexp", &search_regex], - )?; + let output = self + .command_runner + .execute( + "git", + &["config", "--global", "--get-regexp", &search_regex], + ) + .context("Failed to list team members from git config")?; match output.status_code { Some(Self::EXIT_CODE_SUCCESS) => String::from_utf8(output.stdout)? @@ -50,7 +54,7 @@ impl TeamMemberRepo for GitConfigTeamMemberRepo { " ".to_owned() }; x.split_once(&delimiter) - .ok_or(format!("Failed to split string: '{x}'").into()) + .ok_or_else(|| anyhow!("Failed to split string: '{x}'")) .map(|(_, team_member)| team_member.to_owned()) }) .collect(), @@ -65,12 +69,16 @@ impl TeamMemberRepo for GitConfigTeamMemberRepo { let output = self .command_runner - .execute("git", &["config", "--global", &full_key])?; + .execute("git", &["config", "--global", &full_key]) + .with_context(|| format!("Failed to get team member '{key}' from git config"))?; match output.status_code { - Some(Self::EXIT_CODE_SUCCESS) => { - Ok(Some(String::from_utf8(output.stdout)?.trim().into())) - } + Some(Self::EXIT_CODE_SUCCESS) => Ok(Some( + String::from_utf8(output.stdout) + .context("Failed to parse git config output as UTF-8")? + .trim() + .into(), + )), Some(Self::EXIT_CODE_CONFIG_INVALID_KEY) => Ok(None), _ => Self::git_config_error(&output), } @@ -81,7 +89,8 @@ impl TeamMemberRepo for GitConfigTeamMemberRepo { let output = self .command_runner - .execute("git", &["config", "--global", "--unset-all", &full_key])?; + .execute("git", &["config", "--global", "--unset-all", &full_key]) + .with_context(|| format!("Failed to remove team member '{key}' from git config"))?; match output.status_code { Some(Self::EXIT_CODE_SUCCESS) => Ok(()), @@ -94,11 +103,14 @@ impl TeamMemberRepo for GitConfigTeamMemberRepo { let output = self .command_runner - .execute("git", &["config", "--global", &full_key, team_member])?; + .execute("git", &["config", "--global", &full_key, team_member]) + .with_context(|| format!("Failed to add team member '{key}' to git config"))?; match output.status_code { Some(Self::EXIT_CODE_SUCCESS) => Ok(()), - Some(Self::EXIT_CODE_CONFIG_INVALID_KEY) => Err(format!("Invalid key: {key}").into()), + Some(Self::EXIT_CODE_CONFIG_INVALID_KEY) => { + bail!("Invalid key: {key}") + } _ => Self::git_config_error(&output), } } diff --git a/tests/mob.rs b/tests/mob.rs index 7445368..80ff990 100644 --- a/tests/mob.rs +++ b/tests/mob.rs @@ -44,7 +44,7 @@ fn test_mob_with_by_key_when_team_member_not_found( .assert() .failure() .stderr(predicate::str::diff( - "Error: \"No team member found with key: jk\"\n", + "Error: No team member found with key: jk\n", )); Ok(()) @@ -61,7 +61,7 @@ fn test_mob_with_multiselect_given_no_team_members_added( .assert() .failure() .stderr(predicate::str::diff( - "Error: \"No team member(s) found. At least one team member must be added\"\n", + "Error: No team member(s) found. At least one team member must be added\n", )); Ok(()) diff --git a/tests/setup.rs b/tests/setup.rs index cfe6c31..fba15d7 100644 --- a/tests/setup.rs +++ b/tests/setup.rs @@ -190,7 +190,7 @@ fn test_setup_given_invalid_git_config_global_path( .assert() .failure() .stderr(predicate::str::contains( - "Error: \"Failed to set global githooks directory to", + "Error: Failed to set global githooks directory to", )); Ok(()) @@ -203,7 +203,7 @@ fn test_setup_local_given_hooks_dir_not_set(ctx: TestContextRepo) -> Result<(), .args(["mob", "setup", "--local"]) .assert() .failure() - .stderr("Error: \"Local githooks directory is not set\"\n"); + .stderr("Error: Local githooks directory is not set\n"); Ok(()) } diff --git a/tests/team_member.rs b/tests/team_member.rs index 8099edd..7a2cc13 100644 --- a/tests/team_member.rs +++ b/tests/team_member.rs @@ -65,7 +65,7 @@ fn test_add_member_with_invalid_key(ctx: TestContextCli) -> Result<(), Box Result<(), B .assert() .failure() .stderr(predicate::str::diff( - "Error: \"No team member found with key: lm\"\n", + "Error: No team member found with key: lm\n", )); Ok(())