From 441340fc187c11108331f244ca490391b0decc01 Mon Sep 17 00:00:00 2001 From: Matan Zruya Date: Fri, 31 Mar 2023 23:35:44 -0400 Subject: [PATCH 1/2] tidy validator errors --- src/ownership.rs | 2 +- src/ownership/validator.rs | 56 ++++++++++++++++++++++++-------------- 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 3e3a4e8..7df6538 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -10,7 +10,7 @@ mod tests; use crate::project::Project; -pub use validator::ValidationErrors; +pub use validator::Errors as ValidationErrors; use self::{ file_generator::FileGenerator, diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 479004c..b5e37d5 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,6 +1,5 @@ use core::fmt; use std::collections::HashMap; -use std::error::Error; use std::fmt::Display; use std::path::Path; @@ -31,18 +30,18 @@ struct Owner { } #[derive(Debug)] -enum ValidationError { +enum Error { FileWithoutOwner { path: PathBuf }, FileWithMultipleOwners { path: PathBuf, owners: Vec }, CodeownershipFileIsStale, } #[derive(Debug)] -pub struct ValidationErrors(Vec); +pub struct Errors(Vec); impl Validator { #[instrument(level = "debug", skip_all)] - pub fn validate(&self) -> Result<(), ValidationErrors> { + pub fn validate(&self) -> Result<(), Errors> { let mut validation_errors = Vec::new(); debug!("validate_file_ownership"); @@ -54,20 +53,20 @@ impl Validator { if validation_errors.is_empty() { Ok(()) } else { - Err(ValidationErrors(validation_errors)) + Err(Errors(validation_errors)) } } - fn validate_file_ownership(&self) -> Vec { + fn validate_file_ownership(&self) -> Vec { let mut validation_errors = Vec::new(); for (file, owners) in self.file_to_owners() { let relative_path = self.project.relative_path(&file.path).to_owned(); if owners.is_empty() { - validation_errors.push(ValidationError::FileWithoutOwner { path: relative_path }) + validation_errors.push(Error::FileWithoutOwner { path: relative_path }) } else if owners.len() > 1 { - validation_errors.push(ValidationError::FileWithMultipleOwners { + validation_errors.push(Error::FileWithMultipleOwners { path: relative_path, owners, }) @@ -77,11 +76,11 @@ impl Validator { validation_errors } - fn validate_codeowners_file(&self) -> Vec { + fn validate_codeowners_file(&self) -> Vec { let generated_file = self.file_generator.generate_file(); if generated_file != self.project.codeowners_file { - vec![ValidationError::CodeownershipFileIsStale] + vec![Error::CodeownershipFileIsStale] } else { vec![] } @@ -127,12 +126,12 @@ impl Validator { } } -impl ValidationError { +impl Error { pub fn error_category_message(&self) -> String { match self { - ValidationError::FileWithoutOwner { path: _ } => "Some files are missing ownership:".to_owned(), - ValidationError::FileWithMultipleOwners { path: _, owners: _ } => "Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways.".to_owned(), - ValidationError::CodeownershipFileIsStale => { + Error::FileWithoutOwner { path: _ } => "Some files are missing ownership:".to_owned(), + Error::FileWithMultipleOwners { path: _, owners: _ } => "Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways.".to_owned(), + Error::CodeownershipFileIsStale => { "CODEOWNERS out of date. Run `codeownership generate` to update the CODEOWNERS file".to_owned() } } @@ -140,8 +139,8 @@ impl ValidationError { pub fn error_message(&self) -> String { match self { - ValidationError::FileWithoutOwner { path } => format!("- {}", path.to_string_lossy()), - ValidationError::FileWithMultipleOwners { path, owners } => owners + Error::FileWithoutOwner { path } => format!("- {}", path.to_string_lossy()), + Error::FileWithMultipleOwners { path, owners } => owners .iter() .flat_map(|owner| { owner @@ -150,12 +149,29 @@ impl ValidationError { .map(|source| format!("- {} (owner: {}, source: {})", path.to_string_lossy(), owner.team_name, &source)) }) .join("\n"), - ValidationError::CodeownershipFileIsStale => "".to_owned(), + Error::CodeownershipFileIsStale => "".to_owned(), } } } -impl Display for ValidationErrors { +impl Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str(&self.error_message())?; + Ok(()) + } +} + +impl std::error::Error for Error { + fn description(&self) -> &str { + match self { + Error::FileWithoutOwner { path: _ } => "FileWithoutOwner", + Error::FileWithMultipleOwners { path: _, owners: _ } => "FileWithMultipleOwners", + Error::CodeownershipFileIsStale => "FileWithMultipleOwners", + } + } +} + +impl Display for Errors { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let grouped_errors = self.0.iter().into_group_map_by(|error| error.error_category_message()); for (error_category_message, errors) in grouped_errors { @@ -174,8 +190,8 @@ impl Display for ValidationErrors { } } -impl Error for ValidationErrors { +impl std::error::Error for Errors { fn description(&self) -> &str { - "ValidationError" + "Error" } } From 6084929fac919fcdaf8a9eeb62652093bb9dcb1d Mon Sep 17 00:00:00 2001 From: Matan Zruya Date: Sat, 1 Apr 2023 14:18:23 -0400 Subject: [PATCH 2/2] Integrate error-stack for better error messages --- Cargo.lock | 53 +++++++++++++++++++++++++++++ Cargo.toml | 2 ++ src/main.rs | 69 +++++++++++++++++++++++++------------- src/ownership.rs | 1 + src/ownership/validator.rs | 10 +++--- src/project.rs | 45 ++++++++++++++++++++----- 6 files changed, 142 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 769d8d7..aec371c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -51,6 +51,12 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "anyhow" +version = "1.0.70" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7de8ce5e0f9f8d88245311066a578d72b7af3e7088f32783804676302df237e4" + [[package]] name = "assert_cmd" version = "2.0.10" @@ -160,6 +166,7 @@ dependencies = [ "assert_cmd", "clap", "clap_derive", + "error-stack", "glob-match", "itertools", "jwalk", @@ -169,6 +176,7 @@ dependencies = [ "rusty-hook", "serde", "serde_yaml", + "thiserror", "tracing", "tracing-subscriber", ] @@ -304,6 +312,16 @@ dependencies = [ "libc", ] +[[package]] +name = "error-stack" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5f00447f331c7f726db5b8532ebc9163519eed03c6d7c8b73c90b3ff5646ac85" +dependencies = [ + "anyhow", + "rustc_version", +] + [[package]] name = "fsio" version = "0.1.3" @@ -605,6 +623,15 @@ version = "0.6.29" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f162c6dd7b008981e4d40210aca20b4bd0f9b60ca9271061b07f78537722f2e1" +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver", +] + [[package]] name = "rustix" version = "0.37.5" @@ -643,6 +670,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd" +[[package]] +name = "semver" +version = "1.0.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bebd363326d05ec3e2f532ab7660680f3b02130d780c299bca73469d521bc0ed" + [[package]] name = "serde" version = "1.0.159" @@ -725,6 +758,26 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3369f5ac52d5eb6ab48c6b4ffdc8efbcad6b89c765749064ba298f2c68a16a76" +[[package]] +name = "thiserror" +version = "1.0.40" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "978c9a314bd8dc99be594bc3c175faaa9794be04a5a5e153caba6915336cebac" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.40" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f9456a42c5b0d803c8cd86e73dd7cc9edd429499f37a3550d286d5e86720569f" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.11", +] + [[package]] name = "thread_local" version = "1.1.7" diff --git a/Cargo.toml b/Cargo.toml index 56b7814..217799b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [dependencies] clap = { version = "4.2.1", features = ["derive"] } clap_derive = "4.2.0" +error-stack = "0.3.1" glob-match = "0.2.1" itertools = "0.10.5" jwalk = "0.8.1" @@ -14,6 +15,7 @@ rayon = "1.7.0" regex = "1.7.3" serde = { version = "1.0.159", features = ["derive"] } serde_yaml = "0.9.19" +thiserror = "1.0.40" tracing = "0.1.37" tracing-subscriber = { version = "0.3.16", features = ["env-filter"] } diff --git a/src/main.rs b/src/main.rs index d53d563..66b3db3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,11 +1,11 @@ -use ownership::{Ownership, ValidationErrors}; -use tracing::debug; +use ownership::Ownership; use crate::project::Project; use clap::{Parser, Subcommand}; +use core::fmt; +use error_stack::{Context, IntoReport, Result, ResultExt}; use path_clean::PathClean; use std::{ - error::Error, fs::File, path::{Path, PathBuf}, process, @@ -47,64 +47,87 @@ struct Args { } impl Args { - fn absolute_project_root(&self) -> Result { - self.project_root.canonicalize() + fn absolute_project_root(&self) -> Result { + self.project_root.canonicalize().into_report().change_context(Error::Io) } - fn absolute_config_path(&self) -> Result { + fn absolute_config_path(&self) -> Result { Ok(self.absolute_path(&self.config_path)?.clean()) } - fn absolute_codeowners_path(&self) -> Result { + fn absolute_codeowners_path(&self) -> Result { Ok(self.absolute_path(&self.codeowners_file_path)?.clean()) } - fn absolute_path(&self, path: &Path) -> Result { + fn absolute_path(&self, path: &Path) -> Result { Ok(self.absolute_project_root()?.join(path)) } } -fn main() -> Result<(), Box> { +#[derive(Debug)] +enum Error { + CannotBuildProject, + Io, + ValidationErrors, +} + +impl fmt::Display for Error { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Error::CannotBuildProject => fmt.write_str("Error::CannotBuildProject"), + Error::Io => fmt.write_str("Error::Io"), + Error::ValidationErrors => fmt.write_str("Error::ValidationErrors"), + } + } +} + +impl Context for Error {} + +fn main() -> Result<(), Error> { install_logger(); print_validation_errors_to_stdout(cli())?; Ok(()) } -fn cli() -> Result<(), Box> { +fn cli() -> Result<(), Error> { let args = Args::parse(); let config_path = args.absolute_config_path()?; let codeowners_file_path = args.absolute_codeowners_path()?; let project_root = args.absolute_project_root()?; - debug!( - config_path = &config_path.to_str(), - codeowners_file_path = &codeowners_file_path.to_str(), - project_root = &project_root.to_str(), - ); + let config_file = File::open(&config_path) + .into_report() + .change_context(Error::Io) + .attach_printable(format!("{}", config_path.to_string_lossy()))?; + let config = serde_yaml::from_reader(config_file).into_report().change_context(Error::Io)?; - let config = serde_yaml::from_reader(File::open(config_path)?)?; - let ownership = Ownership::build(Project::build(&project_root, &codeowners_file_path, &config)?); + let ownership = + Ownership::build(Project::build(&project_root, &codeowners_file_path, &config).change_context(Error::CannotBuildProject)?); let command = args.command; match command { - Command::Validate => ownership.validate()?, + Command::Validate => ownership.validate().change_context(Error::ValidationErrors)?, Command::Generate => { - std::fs::write(codeowners_file_path, ownership.generate_file())?; + std::fs::write(codeowners_file_path, ownership.generate_file()) + .into_report() + .change_context(Error::Io)?; } Command::GenerateAndValidate => { - std::fs::write(codeowners_file_path, ownership.generate_file())?; - ownership.validate()? + std::fs::write(codeowners_file_path, ownership.generate_file()) + .into_report() + .change_context(Error::Io)?; + ownership.validate().change_context(Error::ValidationErrors)? } } Ok(()) } -fn print_validation_errors_to_stdout(result: Result<(), Box>) -> Result<(), Box> { +fn print_validation_errors_to_stdout(result: Result<(), Error>) -> Result<(), Error> { if let Err(error) = result { - if let Some(validation_errors) = error.downcast_ref::() { + if let Some(validation_errors) = error.downcast_ref::() { println!("{}", validation_errors); process::exit(-1); } else { diff --git a/src/ownership.rs b/src/ownership.rs index 7df6538..c54d370 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -10,6 +10,7 @@ mod tests; use crate::project::Project; +use error_stack::Result; pub use validator::Errors as ValidationErrors; use self::{ diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index b5e37d5..212a120 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -23,6 +23,8 @@ pub struct Validator { pub file_generator: FileGenerator, } +use error_stack::{Context, Report, Result}; + #[derive(Debug)] struct Owner { pub sources: Vec, @@ -53,7 +55,7 @@ impl Validator { if validation_errors.is_empty() { Ok(()) } else { - Err(Errors(validation_errors)) + Err(Report::new(Errors(validation_errors))) } } @@ -190,8 +192,4 @@ impl Display for Errors { } } -impl std::error::Error for Errors { - fn description(&self) -> &str { - "Error" - } -} +impl Context for Errors {} diff --git a/src/project.rs b/src/project.rs index aa06929..538d23f 100644 --- a/src/project.rs +++ b/src/project.rs @@ -1,11 +1,13 @@ +use core::fmt; use std::{ collections::HashMap, - error::Error, fs::File, io::BufRead, path::{Path, PathBuf}, }; +use error_stack::{Context, IntoReport, Result, ResultExt}; + use jwalk::WalkDir; use rayon::prelude::{IntoParallelIterator, ParallelIterator}; use regex::Regex; @@ -109,9 +111,26 @@ mod deserializers { } } +#[derive(Debug)] +pub enum Error { + Io, + SerdeYaml, +} + +impl fmt::Display for Error { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Error::Io => fmt.write_str("Error::Io"), + Error::SerdeYaml => fmt.write_str("Error::SerdeYaml"), + } + } +} + +impl Context for Error {} + impl Project { #[instrument(level = "debug", skip_all)] - pub fn build(base_path: &Path, codeowners_file_path: &Path, config: &Config) -> Result> { + pub fn build(base_path: &Path, codeowners_file_path: &Path, config: &Config) -> Result { debug!("scanning project ({})", base_path.to_string_lossy()); let mut owned_file_paths: Vec = Vec::new(); @@ -120,10 +139,14 @@ impl Project { let mut vendored_gems: Vec = Vec::new(); for entry in WalkDir::new(base_path) { - let entry = entry?; + let entry = entry.into_report().change_context(Error::Io)?; let absolute_path = entry.path(); - let relative_path = absolute_path.strip_prefix(base_path)?.to_owned(); + let relative_path = absolute_path + .strip_prefix(base_path) + .into_report() + .change_context(Error::Io)? + .to_owned(); if entry.file_type().is_dir() { if relative_path.parent() == Some(Path::new(&config.vendored_gems_path)) { @@ -160,7 +183,8 @@ impl Project { } if matches_globs(&relative_path, &config.team_file_glob) { - let deserializer: deserializers::Team = serde_yaml::from_reader(File::open(&absolute_path)?)?; + let file = File::open(&absolute_path).into_report().change_context(Error::Io)?; + let deserializer: deserializers::Team = serde_yaml::from_reader(file).into_report().change_context(Error::SerdeYaml)?; teams.push(Team { path: absolute_path.clone(), @@ -186,7 +210,9 @@ impl Project { ); let codeowners_file: String = if codeowners_file_path.exists() { - std::fs::read_to_string(codeowners_file_path)? + std::fs::read_to_string(codeowners_file_path) + .into_report() + .change_context(Error::Io)? } else { "".to_owned() }; @@ -245,7 +271,7 @@ fn owned_files(owned_file_paths: Vec) -> Vec { .into_par_iter() .map(|path| { let file = File::open(&path).unwrap_or_else(|_| panic!("Couldn't open {}", path.to_string_lossy())); - let first_line: Result, std::io::Error> = std::io::BufReader::new(file).lines().next().transpose(); + let first_line = std::io::BufReader::new(file).lines().next().transpose(); let first_line = first_line.expect("error reading first line"); if first_line.is_none() { @@ -272,8 +298,9 @@ fn owned_files(owned_file_paths: Vec) -> Vec { .collect() } -fn package_owner(path: &Path) -> Result, Box> { - let deserializer: deserializers::Package = serde_yaml::from_reader(File::open(path)?)?; +fn package_owner(path: &Path) -> Result, Error> { + let file = File::open(path).into_report().change_context(Error::Io)?; + let deserializer: deserializers::Package = serde_yaml::from_reader(file).into_report().change_context(Error::SerdeYaml)?; if let Some(metadata) = deserializer.metadata { Ok(metadata.owner)