From 8510ed3e43ff0a92565815b02645cc7bf5cb6b18 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 7 Oct 2021 23:19:31 +0200 Subject: Modernize error handling This sprinkles in some thiserror and anyhow instead of the hand-rolled 'error_froms!' macros and the MainResult. The benefit of this is that we're hooking into an established ecosystem of error handling and we're saving a lot of effort in the hand-written Display and Error implementations. The reason for not sprinkling anyhow everywhere is because the retrieval/rendering part of kondou could be split off into a library at some point, in which case we want to have a proper KondouError type. However, the argument could be made that the current split of errors is not very good, especially because many of them boil down to the same inner errors (RenderError wrapping ApiError wrapping reqwest::Error), which keeps unnecessary information. Some future improvements may include 1.) Unifying those error enums into one bigger enum 2.) Attaching more context through anyhow in the application layer 3.) Properly define an API and split off the inner logic from the UI logic --- Cargo.toml | 2 ++ src/api/mod.rs | 49 ++++++++++------------------------------- src/bt.rs | 40 +++++++++++++++------------------ src/cache.rs | 28 +++++------------------- src/main.rs | 68 +++++++++++++-------------------------------------------- src/output.rs | 5 +++-- src/render.rs | 38 ++++++-------------------------- src/useropts.rs | 39 +++++++-------------------------- 8 files changed, 70 insertions(+), 199 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ec6cf67..df5ca0e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,3 +23,5 @@ num_enum = "0.5.4" num-traits = "0.2" byteorder = "1.3" toml = "0.5" +anyhow = "1.0.44" +thiserror = "1.0.29" diff --git a/src/api/mod.rs b/src/api/mod.rs index 1d069cd..7cec4e6 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -18,51 +18,26 @@ use image::DynamicImage; use itertools::Itertools; use reqwest::{blocking::Client, StatusCode, Url}; use serde::{de::DeserializeOwned, Serialize}; -use std::{error::Error, fmt, path::Path}; +use std::path::Path; +use thiserror::Error; use super::cache::{Cache, CacheError}; /// The base URL of the official Guild Wars 2 API. const BASE_URL: &str = "https://api.guildwars2.com/v2/"; -#[derive(Debug)] +#[derive(Error, Debug)] pub enum ApiError { + #[error("The requested item could not be found in the API")] ItemNotFound, - SerializationError(serde_json::Error), - CacheError(CacheError), - HttpError(reqwest::Error), - ImageError(image::ImageError), -} - -error_froms! { ApiError, - err: serde_json::Error => ApiError::SerializationError(err), - err: CacheError => ApiError::CacheError(err), - err: reqwest::Error => ApiError::HttpError(err), - err: image::ImageError => ApiError::ImageError(err), -} - -impl fmt::Display for ApiError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - ApiError::ItemNotFound => write!(f, "the requested item was not found in the API"), - ApiError::SerializationError(_) => write!(f, "error deserializing the returned value"), - ApiError::CacheError(_) => write!(f, "error accessing the cache"), - ApiError::HttpError(_) => write!(f, "HTTP error"), - ApiError::ImageError(_) => write!(f, "image processing error"), - } - } -} - -impl Error for ApiError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match *self { - ApiError::SerializationError(ref err) => Some(err), - ApiError::CacheError(ref err) => Some(err), - ApiError::HttpError(ref err) => Some(err), - ApiError::ImageError(ref err) => Some(err), - _ => None, - } - } + #[error("Error deserializing the API response")] + SerializationError(#[from] serde_json::Error), + #[error("Error accessing the cache")] + CacheError(#[from] CacheError), + #[error("Underlying HTTP error")] + HttpError(#[from] reqwest::Error), + #[error("Image loading error")] + ImageError(#[from] image::ImageError), } trait ApiResponse diff --git a/src/bt.rs b/src/bt.rs index c379d1d..3c1b604 100644 --- a/src/bt.rs +++ b/src/bt.rs @@ -1,42 +1,38 @@ use super::api::{Api, ApiError, Profession, Skill, Specialization}; use byteorder::{ReadBytesExt, WriteBytesExt, LE}; use num_enum::{IntoPrimitive, TryFromPrimitive}; -use std::{convert::TryFrom, error::Error, fmt, io::Cursor, str::FromStr}; +use std::{convert::TryFrom, fmt, io::Cursor, str::FromStr}; +use thiserror::Error; -#[derive(Debug)] +#[derive(Error, Debug)] pub enum ChatlinkError { - ApiError(ApiError), + #[error("Error accessing the API")] + ApiError(#[from] ApiError), + #[error("The input link is malformed")] MalformedInput, } -error_froms! { ChatlinkError, - err: ApiError => ChatlinkError::ApiError(err), - _err: base64::DecodeError => ChatlinkError::MalformedInput, - _err: num_enum::TryFromPrimitiveError => ChatlinkError::MalformedInput, - _err: num_enum::TryFromPrimitiveError => ChatlinkError::MalformedInput, -} - impl From for ChatlinkError { fn from(_err: std::io::Error) -> Self { panic!("The reading cursor should never return an error!"); } } -impl fmt::Display for ChatlinkError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - ChatlinkError::ApiError(_) => write!(f, "error accessing the API"), - ChatlinkError::MalformedInput => write!(f, "the input link is malformed"), - } +impl From for ChatlinkError { + fn from(_: base64::DecodeError) -> Self { + ChatlinkError::MalformedInput } } -impl Error for ChatlinkError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match *self { - ChatlinkError::ApiError(ref err) => Some(err), - _ => None, - } +impl From> for ChatlinkError { + fn from(_: num_enum::TryFromPrimitiveError) -> Self { + ChatlinkError::MalformedInput + } +} + +impl From> for ChatlinkError { + fn from(_: num_enum::TryFromPrimitiveError) -> Self { + ChatlinkError::MalformedInput } } diff --git a/src/cache.rs b/src/cache.rs index 09c8512..7dd65cf 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -1,33 +1,15 @@ //! Caching support to prevent hitting the API a lot. -use std::{error::Error, fmt, fs, path::Path}; +use std::{fs, path::Path}; +use thiserror::Error; use xdg::BaseDirectories; use super::APP_NAME; -#[derive(Debug)] +#[derive(Error, Debug)] pub enum CacheError { - Io(std::io::Error), -} - -error_froms! { CacheError, - err: std::io::Error => CacheError::Io(err), -} - -impl fmt::Display for CacheError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - CacheError::Io(_) => write!(f, "cache input/output error"), - } - } -} - -impl Error for CacheError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match *self { - CacheError::Io(ref err) => Some(err), - } - } + #[error("Cache I/O error")] + Io(#[from] std::io::Error), } /// A generic cache. diff --git a/src/main.rs b/src/main.rs index 98dbc1e..5b11a40 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,18 +1,3 @@ -use std::error::Error as StdError; -use std::fmt; - -macro_rules! error_froms { - ($tname:ty, $($ename:ident : $fty:ty => $res:expr,)*) => { - $( - impl From<$fty> for $tname { - fn from($ename: $fty) -> Self { - $res - } - } - )* - } -} - mod api; mod bt; mod cache; @@ -20,6 +5,8 @@ mod output; mod render; mod useropts; +use anyhow::{Context, Result}; +use thiserror::Error; use clap::{App, Arg, ArgMatches}; use api::{Api, Profession, Skill}; @@ -32,54 +19,30 @@ use render::RenderError; const APP_NAME: &str = "kondou"; /// Return value indicating that a requested resource could not be found. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Error)] enum NotFound { /// Used when the requested profession can not be found. /// /// The argument is the requested profession. + #[error("The profession '{0}' could not be found")] Profession(String), /// Used when a skill given by its ID could not be found. /// /// The argument is the requested skill id. + #[error("The skill with ID {0} could not be found")] SkillId(u32), /// Used when a skill given by its name could not be found. /// /// The argument is the requested skill name. + #[error("The skill named '{0}' could not be found")] SkillName(String), /// Used when a specialization could not be found. /// /// The argument is the requested specialization. + #[error("The specialization named '{0}' could not be found")] Specialization(String), } -impl fmt::Display for NotFound { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - NotFound::Profession(ref name) => { - write!(f, "the profession '{}' could not be found", name) - } - NotFound::SkillId(id) => write!(f, "the skill with the ID '{}' was not found", id), - NotFound::SkillName(ref name) => { - write!(f, "the skill with the name '{}' was not found", name) - } - NotFound::Specialization(ref name) => write!( - f, - "the specialization with the name '{}' was not found", - name - ), - } - } -} - -impl StdError for NotFound {} - -/// A top-level result. -/// -/// We use a dynamic error dispatch here for two reasons: -/// 1. The only thing that we really do here is displaying the error to the user. -/// 2. We don't want yet another error kind with a lot of kinds. -type MainResult = Result>; - /// A trait for containers that only contain a single item. trait SingleContainer { /// Extract the single element by consuming the container. @@ -94,7 +57,7 @@ impl SingleContainer for Vec { } /// Find the profession by the given name. -fn find_profession(api: &mut Api, name: &str) -> MainResult { +fn find_profession(api: &mut Api, name: &str) -> Result { let profession_ids = api.get_profession_ids()?; let lower_name = name.to_lowercase(); let profession_id = profession_ids @@ -110,7 +73,7 @@ fn find_profession(api: &mut Api, name: &str) -> MainResult { /// /// `text` can either be a skill name, in which case all skills of the profession will be searched. /// Alternatively, it can also be a numeric ID, in which case it will be requested directly. -fn resolve_skill(api: &mut Api, profession: &Profession, text: &str) -> MainResult { +fn resolve_skill(api: &mut Api, profession: &Profession, text: &str) -> Result { // Try it as an ID first let numeric = text.parse::(); if let Ok(num_id) = numeric { @@ -135,7 +98,7 @@ fn resolve_skill(api: &mut Api, profession: &Profession, text: &str) -> MainResu /// Resolve a traitline. /// /// `text` must be in the `"name:choice1:choice2:choice3"` format. -fn resolve_traitline(api: &mut Api, profession: &Profession, text: &str) -> MainResult { +fn resolve_traitline(api: &mut Api, profession: &Profession, text: &str) -> Result { let parts = text.split(':').collect::>(); assert_eq!( parts.len(), @@ -162,7 +125,7 @@ fn resolve_traitline(api: &mut Api, profession: &Profession, text: &str) -> Main } /// Create the build template by manually combining the given skills/traitlines from the CLI. -fn run_searching(api: &mut Api, matches: &ArgMatches) -> MainResult { +fn run_searching(api: &mut Api, matches: &ArgMatches) -> Result { let requested_profession = matches .value_of("profession") .expect("clap handles missing argument"); @@ -229,7 +192,7 @@ fn run_searching(api: &mut Api, matches: &ArgMatches) -> MainResult MainResult { +fn run_chatlink(api: &mut Api, matches: &ArgMatches) -> Result { let link = matches.value_of("chatlink").unwrap(); Ok(BuildTemplate::from_chatlink(api, link)?) } @@ -262,7 +225,7 @@ fn validate_legend(input: String) -> Result<(), String> { .map_err(|_| "invalid legend name".to_owned()) } -fn run() -> MainResult<()> { +fn run() -> Result<()> { let matches = App::new(APP_NAME) .version("0.1") .author("Peter Parker IV") @@ -375,8 +338,7 @@ fn run() -> MainResult<()> { } Err(RenderError::EmptyBuild) => (), Err(err) => { - eprintln!("Image could not be rendered:"); - output::show_error(&err)?; + return Err(err).context("Image could not be rendered"); } } @@ -386,7 +348,7 @@ fn run() -> MainResult<()> { fn main() { let result = run(); if let Err(e) = result { - output::show_error(e.as_ref()).expect("Error while displaying error"); + output::show_error(e).expect("Error while displaying error"); std::process::exit(1); } } diff --git a/src/output.rs b/src/output.rs index 8d71909..c00d4bb 100644 --- a/src/output.rs +++ b/src/output.rs @@ -3,8 +3,9 @@ use super::{ api, bt::{BuildTemplate, Traitline}, }; -use std::{error::Error, io, io::Write}; +use std::{io, io::Write}; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; +use anyhow::Error; const HEADER_COLOR: Color = Color::Cyan; @@ -67,7 +68,7 @@ pub fn show_build_template(build: &BuildTemplate) -> io::Result<()> { /// Show an error to the standard error stream. /// /// This will also show the chain of errors that lead up to this error, if available. -pub fn show_error(error: &E) -> io::Result<()> { +pub fn show_error(error: Error) -> io::Result<()> { let mut error_color = ColorSpec::new(); error_color.set_fg(Some(Color::Red)); let mut stderr = StandardStream::stderr(ColorChoice::Auto); diff --git a/src/render.rs b/src/render.rs index af5d2b2..484fd83 100644 --- a/src/render.rs +++ b/src/render.rs @@ -8,42 +8,18 @@ use imageproc::{drawing, rect::Rect}; use num_traits::{Num, NumCast}; use rusttype::{Font, Scale}; use serde::{Deserialize, Serialize}; -use std::{error::Error, fmt}; +use thiserror::Error; -#[derive(Debug)] +#[derive(Error, Debug)] pub enum RenderError { - ApiError(ApiError), - ImageError(image::ImageError), + #[error("Error accessing the API")] + ApiError(#[from] ApiError), + #[error("Image processing error")] + ImageError(#[from] image::ImageError), + #[error("The build template contains nothing worth rendering")] EmptyBuild, } -error_froms! { RenderError, - err: ApiError => RenderError::ApiError(err), - err: image::ImageError => RenderError::ImageError(err), -} - -impl fmt::Display for RenderError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match *self { - RenderError::ApiError(_) => write!(f, "error accessing the API"), - RenderError::ImageError(_) => write!(f, "image processing error"), - RenderError::EmptyBuild => { - write!(f, "the build template contains nothing worth rendering") - } - } - } -} - -impl Error for RenderError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match *self { - RenderError::ApiError(ref err) => Some(err), - RenderError::ImageError(ref err) => Some(err), - _ => None, - } - } -} - #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub enum Alignment { Left, diff --git a/src/useropts.rs b/src/useropts.rs index 27e14df..2e38f05 100644 --- a/src/useropts.rs +++ b/src/useropts.rs @@ -11,45 +11,22 @@ use image::Rgba; use rusttype::Font; use serde::{Deserialize, Serialize}; -use std::{error::Error, fmt, fs, io, path::Path}; +use std::{fs, io, path::Path}; +use thiserror::Error; use super::render::{Alignment, RenderOptions}; /// Error that can occur during loading or converting user options. -#[derive(Debug)] +#[derive(Error, Debug)] pub enum ConfigError { - Io(io::Error), - Serialization(toml::de::Error), + #[error("I/O error")] + Io(#[from] io::Error), + #[error("Deserialization error")] + Serialization(#[from] toml::de::Error), + #[error("Font loading error")] Font, } -error_froms! { - ConfigError, - err: io::Error => ConfigError::Io(err), - err: toml::de::Error => ConfigError::Serialization(err), -} - -impl fmt::Display for ConfigError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "ConfigError: ")?; - match *self { - ConfigError::Io(_) => write!(f, "input/output error"), - ConfigError::Serialization(_) => write!(f, "serialization error"), - ConfigError::Font => write!(f, "could not load the font"), - } - } -} - -impl Error for ConfigError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - match *self { - ConfigError::Io(ref err) => Some(err), - ConfigError::Serialization(ref err) => Some(err), - ConfigError::Font => None, - } - } -} - macro_rules! maybe_take_from { (from: $from:expr, to: $to:ident, $($field:ident,)*) => { $( -- cgit v1.2.3