From bb7130f96041e5c22e4559374dd5fdb9a6cd3190 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Mon, 3 May 2021 17:57:04 -0400 Subject: [PATCH] Support multiple exit codes ..with more specific error enums. --- Cargo.lock | 1 + POLICY.md | 10 +++--- cli/Cargo.toml | 1 + cli/src/argparse/command.rs | 18 +++++++--- cli/src/bin/ta.rs | 9 +++-- cli/src/errors.rs | 59 +++++++++++++++++++++++++++++++ cli/src/invocation/cmd/add.rs | 2 +- cli/src/invocation/cmd/config.rs | 2 +- cli/src/invocation/cmd/gc.rs | 2 +- cli/src/invocation/cmd/help.rs | 2 +- cli/src/invocation/cmd/info.rs | 2 +- cli/src/invocation/cmd/modify.rs | 2 +- cli/src/invocation/cmd/report.rs | 2 +- cli/src/invocation/cmd/sync.rs | 2 +- cli/src/invocation/cmd/version.rs | 2 +- cli/src/invocation/mod.rs | 2 +- cli/src/invocation/report.rs | 2 +- cli/src/lib.rs | 7 ++-- cli/tests/cli.rs | 3 +- taskchampion/src/errors.rs | 7 ++-- taskchampion/src/lib.rs | 1 + taskchampion/src/replica.rs | 2 +- taskchampion/src/taskdb.rs | 6 ++-- 23 files changed, 112 insertions(+), 34 deletions(-) create mode 100644 cli/src/errors.rs diff --git a/Cargo.lock b/Cargo.lock index 8a50e2d2a..6853f373a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2176,6 +2176,7 @@ dependencies = [ "tempfile", "termcolor", "textwrap 0.13.4", + "thiserror", "toml", "toml_edit", ] diff --git a/POLICY.md b/POLICY.md index 9f9af590b..fb673155d 100644 --- a/POLICY.md +++ b/POLICY.md @@ -35,12 +35,10 @@ Considered to be part of the API policy. ## CLI exit codes -- `0` No errors, normal exit. -- `1` Generic error. -- `2` Never used to avoid conflicts with Bash. -- `3` Unable to execute with the given parameters. -- `4` I/O error. -- `5` Database error. +- `0` - No errors, normal exit. +- `1` - Generic error. +- `2` - Never used to avoid conflicts with Bash. +- `3` - Command-line Syntax Error. # Security diff --git a/cli/Cargo.toml b/cli/Cargo.toml index d66cc63b9..a3a5ad20c 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -8,6 +8,7 @@ version = "0.3.0" dirs-next = "^2.0.0" env_logger = "^0.8.3" anyhow = "1.0" +thiserror = "1.0" log = "^0.4.14" nom = "^6.1.2" prettytable-rs = "^0.8.0" diff --git a/cli/src/argparse/command.rs b/cli/src/argparse/command.rs index 79a715ec7..891a4525f 100644 --- a/cli/src/argparse/command.rs +++ b/cli/src/argparse/command.rs @@ -1,6 +1,5 @@ use super::args::*; use super::{ArgList, Subcommand}; -use anyhow::bail; use nom::{combinator::*, sequence::*, Err, IResult}; /// A command is the overall command that the CLI should execute. @@ -29,13 +28,22 @@ impl Command { } /// Parse a command from the given list of strings. - pub fn from_argv(argv: &[&str]) -> anyhow::Result { + pub fn from_argv(argv: &[&str]) -> Result { match Command::parse(argv) { Ok((&[], cmd)) => Ok(cmd), - Ok((trailing, _)) => bail!("command line has trailing arguments: {:?}", trailing), + Ok((trailing, _)) => Err(crate::Error::for_arguments(format!( + "command line has trailing arguments: {:?}", + trailing + ))), Err(Err::Incomplete(_)) => unreachable!(), - Err(Err::Error(e)) => bail!("command line not recognized: {:?}", e), - Err(Err::Failure(e)) => bail!("command line not recognized: {:?}", e), + Err(Err::Error(e)) => Err(crate::Error::for_arguments(format!( + "command line not recognized: {:?}", + e + ))), + Err(Err::Failure(e)) => Err(crate::Error::for_arguments(format!( + "command line not recognized: {:?}", + e + ))), } } } diff --git a/cli/src/bin/ta.rs b/cli/src/bin/ta.rs index ecf529be3..efdee99da 100644 --- a/cli/src/bin/ta.rs +++ b/cli/src/bin/ta.rs @@ -1,8 +1,11 @@ use std::process::exit; pub fn main() { - if let Err(err) = taskchampion_cli::main() { - eprintln!("{:?}", err); - exit(1); + match taskchampion_cli::main() { + Ok(_) => exit(0), + Err(e) => { + eprintln!("{:?}", e); + exit(e.exit_status()); + } } } diff --git a/cli/src/errors.rs b/cli/src/errors.rs new file mode 100644 index 000000000..16ac96285 --- /dev/null +++ b/cli/src/errors.rs @@ -0,0 +1,59 @@ +use taskchampion::Error as TcError; +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum Error { + #[error("Command-Line Syntax Error: {0}")] + Arguments(String), + + #[error(transparent)] + TaskChampion(#[from] TcError), + + #[error(transparent)] + Other(#[from] anyhow::Error), +} + +impl Error { + /// Construct a new command-line argument error + pub(crate) fn for_arguments(msg: S) -> Self { + Error::Arguments(msg.to_string()) + } + + /// Determine the exit status for this error, as documented. + pub fn exit_status(&self) -> i32 { + match *self { + Error::Arguments(_) => 3, + _ => 1, + } + } +} + +impl From for Error { + fn from(err: std::io::Error) -> Self { + let err: anyhow::Error = err.into(); + Error::Other(err) + } +} + +#[cfg(test)] +mod test { + use super::*; + use anyhow::anyhow; + + #[test] + fn test_exit_status() { + let mut err: Error; + + err = anyhow!("uhoh").into(); + assert_eq!(err.exit_status(), 1); + + err = Error::Arguments("uhoh".to_string()); + assert_eq!(err.exit_status(), 3); + + err = std::io::Error::last_os_error().into(); + assert_eq!(err.exit_status(), 1); + + err = TcError::Database("uhoh".to_string()).into(); + assert_eq!(err.exit_status(), 1); + } +} diff --git a/cli/src/invocation/cmd/add.rs b/cli/src/invocation/cmd/add.rs index 8a4456282..abee1bf4d 100644 --- a/cli/src/invocation/cmd/add.rs +++ b/cli/src/invocation/cmd/add.rs @@ -6,7 +6,7 @@ pub(crate) fn execute( w: &mut W, replica: &mut Replica, modification: Modification, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { let description = match modification.description { DescriptionMod::Set(ref s) => s.clone(), _ => "(no description)".to_owned(), diff --git a/cli/src/invocation/cmd/config.rs b/cli/src/invocation/cmd/config.rs index 0f34defae..2b57aa52d 100644 --- a/cli/src/invocation/cmd/config.rs +++ b/cli/src/invocation/cmd/config.rs @@ -6,7 +6,7 @@ pub(crate) fn execute( w: &mut W, config_operation: ConfigOperation, settings: &Settings, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { match config_operation { ConfigOperation::Set(key, value) => { let filename = settings.set(&key, &value)?; diff --git a/cli/src/invocation/cmd/gc.rs b/cli/src/invocation/cmd/gc.rs index 775b3096f..9b14b9fbb 100644 --- a/cli/src/invocation/cmd/gc.rs +++ b/cli/src/invocation/cmd/gc.rs @@ -1,7 +1,7 @@ use taskchampion::Replica; use termcolor::WriteColor; -pub(crate) fn execute(w: &mut W, replica: &mut Replica) -> anyhow::Result<()> { +pub(crate) fn execute(w: &mut W, replica: &mut Replica) -> Result<(), crate::Error> { log::debug!("rebuilding working set"); replica.rebuild_working_set(true)?; writeln!(w, "garbage collected.")?; diff --git a/cli/src/invocation/cmd/help.rs b/cli/src/invocation/cmd/help.rs index 421c140a7..2f81a08f8 100644 --- a/cli/src/invocation/cmd/help.rs +++ b/cli/src/invocation/cmd/help.rs @@ -5,7 +5,7 @@ pub(crate) fn execute( w: &mut W, command_name: String, summary: bool, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { let usage = Usage::new(); usage.write_help(w, command_name.as_ref(), summary)?; Ok(()) diff --git a/cli/src/invocation/cmd/info.rs b/cli/src/invocation/cmd/info.rs index 1f90b79f3..5d26213d1 100644 --- a/cli/src/invocation/cmd/info.rs +++ b/cli/src/invocation/cmd/info.rs @@ -10,7 +10,7 @@ pub(crate) fn execute( replica: &mut Replica, filter: Filter, debug: bool, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { let working_set = replica.working_set()?; for task in filtered_tasks(replica, &filter)? { diff --git a/cli/src/invocation/cmd/modify.rs b/cli/src/invocation/cmd/modify.rs index 06b4ccc0e..6f17f0dba 100644 --- a/cli/src/invocation/cmd/modify.rs +++ b/cli/src/invocation/cmd/modify.rs @@ -8,7 +8,7 @@ pub(crate) fn execute( replica: &mut Replica, filter: Filter, modification: Modification, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { for task in filtered_tasks(replica, &filter)? { let mut task = task.into_mut(replica); diff --git a/cli/src/invocation/cmd/report.rs b/cli/src/invocation/cmd/report.rs index 7123f0353..ab079af28 100644 --- a/cli/src/invocation/cmd/report.rs +++ b/cli/src/invocation/cmd/report.rs @@ -10,7 +10,7 @@ pub(crate) fn execute( settings: &Settings, report_name: String, filter: Filter, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { display_report(w, replica, settings, report_name, filter) } diff --git a/cli/src/invocation/cmd/sync.rs b/cli/src/invocation/cmd/sync.rs index 2e9400642..ce213a5ba 100644 --- a/cli/src/invocation/cmd/sync.rs +++ b/cli/src/invocation/cmd/sync.rs @@ -5,7 +5,7 @@ pub(crate) fn execute( w: &mut W, replica: &mut Replica, server: &mut Box, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { replica.sync(server)?; writeln!(w, "sync complete.")?; Ok(()) diff --git a/cli/src/invocation/cmd/version.rs b/cli/src/invocation/cmd/version.rs index baef94161..5ff2fea57 100644 --- a/cli/src/invocation/cmd/version.rs +++ b/cli/src/invocation/cmd/version.rs @@ -1,6 +1,6 @@ use termcolor::{ColorSpec, WriteColor}; -pub(crate) fn execute(w: &mut W) -> anyhow::Result<()> { +pub(crate) fn execute(w: &mut W) -> Result<(), crate::Error> { write!(w, "TaskChampion ")?; w.set_color(ColorSpec::new().set_bold(true))?; writeln!(w, "{}", env!("CARGO_PKG_VERSION"))?; diff --git a/cli/src/invocation/mod.rs b/cli/src/invocation/mod.rs index a9723fe9a..985c4d99e 100644 --- a/cli/src/invocation/mod.rs +++ b/cli/src/invocation/mod.rs @@ -19,7 +19,7 @@ use report::display_report; /// Invoke the given Command in the context of the given settings #[allow(clippy::needless_return)] -pub(crate) fn invoke(command: Command, settings: Settings) -> anyhow::Result<()> { +pub(crate) fn invoke(command: Command, settings: Settings) -> Result<(), crate::Error> { log::debug!("command: {:?}", command); log::debug!("settings: {:?}", settings); diff --git a/cli/src/invocation/report.rs b/cli/src/invocation/report.rs index 36d15574a..512866381 100644 --- a/cli/src/invocation/report.rs +++ b/cli/src/invocation/report.rs @@ -80,7 +80,7 @@ pub(super) fn display_report( settings: &Settings, report_name: String, filter: Filter, -) -> anyhow::Result<()> { +) -> Result<(), crate::Error> { let mut t = Table::new(); let working_set = replica.working_set()?; diff --git a/cli/src/lib.rs b/cli/src/lib.rs index 2890e4b9f..6996e55f6 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -38,23 +38,26 @@ use std::string::FromUtf8Error; mod macros; mod argparse; +mod errors; mod invocation; mod settings; mod table; mod usage; +pub(crate) use errors::Error; use settings::Settings; /// The main entry point for the command-line interface. This builds an Invocation /// from the particulars of the operating-system interface, and then executes it. -pub fn main() -> anyhow::Result<()> { +pub fn main() -> Result<(), Error> { env_logger::init(); // parse the command line into a vector of &str, failing if // there are invalid utf-8 sequences. let argv: Vec = std::env::args_os() .map(|oss| String::from_utf8(oss.into_vec())) - .collect::>()?; + .collect::>() + .map_err(|_| Error::for_arguments("arguments must be valid utf-8"))?; let argv: Vec<&str> = argv.iter().map(|s| s.as_ref()).collect(); // parse the command line diff --git a/cli/tests/cli.rs b/cli/tests/cli.rs index 7eabe2c77..6325a6d3e 100644 --- a/cli/tests/cli.rs +++ b/cli/tests/cli.rs @@ -56,7 +56,8 @@ fn invalid_option() -> Result<(), Box> { cmd.arg("--no-such-option"); cmd.assert() .failure() - .stderr(predicate::str::contains("command line not recognized")); + .stderr(predicate::str::contains("command line not recognized")) + .code(predicate::eq(3)); Ok(()) } diff --git a/taskchampion/src/errors.rs b/taskchampion/src/errors.rs index 9e2a712a5..44bad9881 100644 --- a/taskchampion/src/errors.rs +++ b/taskchampion/src/errors.rs @@ -1,6 +1,9 @@ use thiserror::Error; + #[derive(Debug, Error, Eq, PartialEq, Clone)] +#[non_exhaustive] +/// Errors returned from taskchampion operations pub enum Error { - #[error("Task Database Error: {}", _0)] - DbError(String), + #[error("Task Database Error: {0}")] + Database(String), } diff --git a/taskchampion/src/lib.rs b/taskchampion/src/lib.rs index da61235d9..a05b1ab7b 100644 --- a/taskchampion/src/lib.rs +++ b/taskchampion/src/lib.rs @@ -40,6 +40,7 @@ mod taskdb; mod utils; mod workingset; +pub use errors::Error; pub use replica::Replica; pub use server::{Server, ServerConfig}; pub use storage::StorageConfig; diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index 1e0a47382..361476951 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -113,7 +113,7 @@ impl Replica { // check that it already exists; this is a convenience check, as the task may already exist // when this Create operation is finally sync'd with operations from other replicas if self.taskdb.get_task(uuid)?.is_none() { - return Err(Error::DbError(format!("Task {} does not exist", uuid)).into()); + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); } self.taskdb.apply(Operation::Delete { uuid })?; trace!("task {} deleted", uuid); diff --git a/taskchampion/src/taskdb.rs b/taskchampion/src/taskdb.rs index ef5fb6c8d..b373bc582 100644 --- a/taskchampion/src/taskdb.rs +++ b/taskchampion/src/taskdb.rs @@ -49,12 +49,12 @@ impl TaskDb { Operation::Create { uuid } => { // insert if the task does not already exist if !txn.create_task(*uuid)? { - return Err(Error::DbError(format!("Task {} already exists", uuid)).into()); + return Err(Error::Database(format!("Task {} already exists", uuid)).into()); } } Operation::Delete { ref uuid } => { if !txn.delete_task(*uuid)? { - return Err(Error::DbError(format!("Task {} does not exist", uuid)).into()); + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); } } Operation::Update { @@ -71,7 +71,7 @@ impl TaskDb { }; txn.set_task(*uuid, task)?; } else { - return Err(Error::DbError(format!("Task {} does not exist", uuid)).into()); + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); } } }