From f5dd866ed4233d0eef926785cb7dbdaeb920cf90 Mon Sep 17 00:00:00 2001 From: Chuck Pierce Date: Fri, 22 Mar 2019 15:29:31 -0700 Subject: [PATCH 1/8] Refactor error formatting to output strings --- crates/notion-core/src/error.rs | 33 +++++++++- crates/notion-core/src/style.rs | 110 +++++++++++++++++--------------- src/cli.rs | 2 - src/main.rs | 9 ++- src/shim.rs | 2 +- 5 files changed, 99 insertions(+), 57 deletions(-) diff --git a/crates/notion-core/src/error.rs b/crates/notion-core/src/error.rs index ab09cc7d8..ec535eaa7 100644 --- a/crates/notion-core/src/error.rs +++ b/crates/notion-core/src/error.rs @@ -1,12 +1,25 @@ +use std::env; use std::fmt; use std::process::ExitStatus; use failure::Fail; -use notion_fail::{ExitCode, NotionFail}; +use notion_fail::{ExitCode, NotionError, NotionFail}; +use crate::style::{format_error_details, format_error_message}; use crate::tool::ToolSpec; use crate::version::VersionSpec; +const NOTION_DEV: &'static str = "NOTION_DEV"; + +/// Represents the context from which an error is being reported. +pub enum ErrorContext { + /// An error reported from the `notion` executable. + Notion, + + /// An error reported from a shim. + Shim, +} + #[derive(Debug, Fail)] pub enum ErrorDetails { /// Thrown when package tries to install a binary that is already installed. @@ -294,3 +307,21 @@ impl NotionFail for ErrorDetails { true } } + +pub fn display_error(cx: ErrorContext, err: &NotionError) { + let message = format_error_message(cx, err); + + eprint!("{}", message); + + if env::var(NOTION_DEV).is_ok() { + let details = format_error_details(err); + eprint!("{}", details); + } +} + +pub fn display_verbose_error(cx: ErrorContext, err: &NotionError) { + let message = format_error_message(cx, err); + let details = format_error_details(err); + + eprint!("{}{}", message, details); +} diff --git a/crates/notion-core/src/style.rs b/crates/notion-core/src/style.rs index 448ce6df0..9aa4dfd94 100644 --- a/crates/notion-core/src/style.rs +++ b/crates/notion-core/src/style.rs @@ -1,94 +1,102 @@ //! The view layer of Notion, with utilities for styling command-line output. use std::env; +use std::fmt::Write; +use crate::error::ErrorContext; use archive::Origin; use console::style; use indicatif::{ProgressBar, ProgressStyle}; use notion_fail::NotionError; use term_size; -const NOTION_DEV: &'static str = "NOTION_DEV"; +// ISSUE #306 - When unknown error messages are removed, this can be removed as well +const INTERNAL_ERROR_MESSAGE: &'static str = "an internal error occurred -/// Represents the context from which an error is being reported. -pub enum ErrorContext { - /// An error reported from the `notion` executable. - Notion, +Notion is still a pre-alpha project, so we expect to run into some bugs, +but we'd love to hear about them so we can fix them! - /// An error reported from a shim. - Shim, +Please feel free to reach out to us at \x1b[36m\x1b[1m@notionjs\x1b[0m on Twitter or file an issue at: + + \x1b[1mhttps://github.com/notion-cli/notion/issues\x1b[0m +"; + +macro_rules! write_str { + ( $( $x:expr ),* ) => { + write!($( $x, )*).expect("write! with String cannot fail") + } } -/// Displays an error to stderr. -pub fn display_error(cx: ErrorContext, err: &NotionError) { - display_error_prefix(cx); +macro_rules! writeln_str { + ( $( $x:expr ),* ) => { + writeln!($( $x, )*).expect("write! with String cannot fail") + } +} + +/// Formats the error message to a string +pub(crate) fn format_error_message(cx: ErrorContext, err: &NotionError) -> String { + let mut message = String::with_capacity(100); + + format_error_prefix(&mut message, cx); if err.is_user_friendly() { - display_user_friendly_error(err); + writeln_str!(message, "{}", err); } else { - display_internal_error(err); + writeln_str!(message, "{}", INTERNAL_ERROR_MESSAGE); } + + message } -/// Displays a user-friendly error to stderr -fn display_user_friendly_error(err: &NotionError) { - eprintln!("{}", err); +/// Formats verbose error details to string +pub(crate) fn format_error_details(err: &NotionError) -> String { + let mut details = String::new(); - if env::var(NOTION_DEV).is_ok() { - eprintln!(); - display_development_details(err); - } + format_error_cause(&mut details, err); + format_error_backtrace(&mut details, err); + + details } -/// Displays an error to stderr with a styled prefix. -fn display_error_prefix(cx: ErrorContext) { +/// Formats a styled prefix for an error +fn format_error_prefix(msg: &mut String, cx: ErrorContext) { match cx { ErrorContext::Notion => { // Since the command here was `notion`, it would be redundant to say that this was // a Notion error, so we are less explicit in the heading. - eprint!("{} ", style("error:").red().bold()); + write_str!(msg, "{} ", style("error:").red().bold()); } ErrorContext::Shim => { // Since a Notion error is rare case for a shim, it can be surprising to a user. // To make it extra clear that this was a failure that happened in Notion when // attempting to delegate to a shim, we are more explicit about the fact that it's // a Notion error. - eprint!("{} ", style("Notion error:").red().bold()); + write_str!(msg, "{} ", style("Notion error:").red().bold()); } } } -/// Displays a generic message for internal errors to stderr. -fn display_internal_error(err: &NotionError) { - eprintln!("an internal error occurred"); - eprintln!(); - - if env::var(NOTION_DEV).is_ok() { - display_development_details(err); - } else { - eprintln!("Notion is still a pre-alpha project, so we expect to run into some bugs,"); - eprintln!("but we'd love to hear about them so we can fix them!"); - eprintln!(); - eprintln!( - "Please feel free to reach out to us at {} on Twitter or file an issue at:", - style("@notionjs").cyan().bold() - ); - eprintln!(); - eprintln!( - " {}", - style("https://github.com/notion-cli/notion/issues").bold() +/// Formats the underlying cause of an error, if it exists +fn format_error_cause(msg: &mut String, err: &NotionError) { + if let Some(inner) = err.as_fail().cause() { + writeln_str!(msg); + write_str!( + msg, + "{}{} ", + style("cause").bold().underlined(), + style(":").bold() ); - eprintln!(); + writeln_str!(msg, "{}", inner); } } -fn display_development_details(err: &NotionError) { - eprintln!("{} {:?}", style("details:").yellow().bold(), err); - eprintln!(); - - // If `RUST_BACKTRACE` is set, then the backtrace will be included in the above output - // If not, we should let the user know how to see the backtrace - if env::var("RUST_BACKTRACE").is_err() { - eprintln!("Run with NOTION_DEV=1 and RUST_BACKTRACE=1 for a backtrace."); +/// Formats the backtrace for an error, if available +fn format_error_backtrace(msg: &mut String, err: &NotionError) { + // ISSUE #75 - Once we have a way to determine backtraces without RUST_BACKTRACE, we can make this always available + // Until then, we know that if the env var is not set, the backtrace will be empty + if env::var("RUST_BACKTRACE").is_ok() { + writeln_str!(msg); + // Note: The implementation of `Display` for Backtrace includes a 'stack backtrace:' prefix + writeln_str!(msg, "{}", err.backtrace()); } } diff --git a/src/cli.rs b/src/cli.rs index 64a3ce127..ade07a48e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -24,9 +24,7 @@ pub(crate) struct Notion { #[structopt(subcommand)] pub(crate) command: Option, - // not yet implemented! #[structopt(long = "verbose", help = "Enables verbose diagnostics", global = true)] - #[allow(dead_code)] pub(crate) verbose: bool, #[structopt( diff --git a/src/main.rs b/src/main.rs index a3493579a..b22c74dac 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,8 +4,8 @@ mod cli; use structopt::StructOpt; +use notion_core::error::{display_error, display_verbose_error, ErrorContext}; use notion_core::session::{ActivityKind, Session}; -use notion_core::style::{display_error, ErrorContext}; /// The entry point for the `notion` CLI. pub fn main() { @@ -14,8 +14,13 @@ pub fn main() { session.add_event_start(ActivityKind::Notion); let notion = cli::Notion::from_args(); + let verbose = notion.verbose; let exit_code = notion.run(&mut session).unwrap_or_else(|err| { - display_error(ErrorContext::Notion, &err); + if verbose { + display_verbose_error(ErrorContext::Notion, &err); + } else { + display_error(ErrorContext::Notion, &err); + } session.add_event_error(ActivityKind::Notion, &err); err.exit_code() }); diff --git a/src/shim.rs b/src/shim.rs index ec21e9b76..9286c8f0f 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -1,5 +1,5 @@ +use notion_core::error::{display_error, ErrorContext}; use notion_core::session::{ActivityKind, Session}; -use notion_core::style::{display_error, ErrorContext}; use notion_core::tool::execute_tool; use notion_fail::ExitCode; From 5146e835c481a7f5347b86763a4d1a889b0831d5 Mon Sep 17 00:00:00 2001 From: Charles Pierce Date: Sat, 23 Mar 2019 09:19:56 -0700 Subject: [PATCH 2/8] Add reporter for outputting errors --- .../src/{error.rs => error/details.rs} | 33 +--------- crates/notion-core/src/error/mod.rs | 5 ++ crates/notion-core/src/error/reporter.rs | 61 +++++++++++++++++++ src/main.rs | 11 ++-- src/shim.rs | 4 +- 5 files changed, 75 insertions(+), 39 deletions(-) rename crates/notion-core/src/{error.rs => error/details.rs} (90%) create mode 100644 crates/notion-core/src/error/mod.rs create mode 100644 crates/notion-core/src/error/reporter.rs diff --git a/crates/notion-core/src/error.rs b/crates/notion-core/src/error/details.rs similarity index 90% rename from crates/notion-core/src/error.rs rename to crates/notion-core/src/error/details.rs index ec535eaa7..ab09cc7d8 100644 --- a/crates/notion-core/src/error.rs +++ b/crates/notion-core/src/error/details.rs @@ -1,25 +1,12 @@ -use std::env; use std::fmt; use std::process::ExitStatus; use failure::Fail; -use notion_fail::{ExitCode, NotionError, NotionFail}; +use notion_fail::{ExitCode, NotionFail}; -use crate::style::{format_error_details, format_error_message}; use crate::tool::ToolSpec; use crate::version::VersionSpec; -const NOTION_DEV: &'static str = "NOTION_DEV"; - -/// Represents the context from which an error is being reported. -pub enum ErrorContext { - /// An error reported from the `notion` executable. - Notion, - - /// An error reported from a shim. - Shim, -} - #[derive(Debug, Fail)] pub enum ErrorDetails { /// Thrown when package tries to install a binary that is already installed. @@ -307,21 +294,3 @@ impl NotionFail for ErrorDetails { true } } - -pub fn display_error(cx: ErrorContext, err: &NotionError) { - let message = format_error_message(cx, err); - - eprint!("{}", message); - - if env::var(NOTION_DEV).is_ok() { - let details = format_error_details(err); - eprint!("{}", details); - } -} - -pub fn display_verbose_error(cx: ErrorContext, err: &NotionError) { - let message = format_error_message(cx, err); - let details = format_error_details(err); - - eprint!("{}{}", message, details); -} diff --git a/crates/notion-core/src/error/mod.rs b/crates/notion-core/src/error/mod.rs new file mode 100644 index 000000000..05986f27e --- /dev/null +++ b/crates/notion-core/src/error/mod.rs @@ -0,0 +1,5 @@ +mod details; +mod reporter; + +pub use details::ErrorDetails; +pub use reporter::{ErrorContext, ErrorReporter}; diff --git a/crates/notion-core/src/error/reporter.rs b/crates/notion-core/src/error/reporter.rs new file mode 100644 index 000000000..747ad2db1 --- /dev/null +++ b/crates/notion-core/src/error/reporter.rs @@ -0,0 +1,61 @@ +use std::env; +use std::io::Result; +use std::path::PathBuf; + +use crate::style::{format_error_details, format_error_message}; +use notion_fail::NotionError; + +const NOTION_DEV: &'static str = "NOTION_DEV"; + +/// Represents the context from which an error is being reported. +pub enum ErrorContext { + /// An error reported from the `notion` executable. + Notion, + + /// An error reported from a shim. + Shim, +} + +pub enum ErrorReporter { + /// Reports errors in the standard, concise format + Standard, + + /// Reports errors with additional details + Verbose, +} + +impl ErrorReporter { + /// Create a new ErrorReporter of the default type + pub fn new() -> Self { + if env::var(NOTION_DEV).is_ok() { + ErrorReporter::Verbose + } else { + ErrorReporter::Standard + } + } + + /// Create a new verbose ErrorReporter + pub fn verbose() -> Self { + ErrorReporter::Verbose + } + + /// Report an error, both to the terminal and the error log + pub fn report(&self, cx: ErrorContext, err: &NotionError) { + let message = format_error_message(cx, err); + let details = format_error_details(err); + + match self { + ErrorReporter::Standard => eprint!("{}", message), + ErrorReporter::Verbose => eprint!("{}{}", message, details), + } + + match write_error_log(message, details) { + Ok(_log_file) => {} + Err(_) => {} + } + } +} + +fn write_error_log(_message: String, _details: String) -> Result { + Ok(PathBuf::from("")) +} diff --git a/src/main.rs b/src/main.rs index b22c74dac..cb9c01895 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,7 +4,7 @@ mod cli; use structopt::StructOpt; -use notion_core::error::{display_error, display_verbose_error, ErrorContext}; +use notion_core::error::{ErrorContext, ErrorReporter}; use notion_core::session::{ActivityKind, Session}; /// The entry point for the `notion` CLI. @@ -16,11 +16,12 @@ pub fn main() { let notion = cli::Notion::from_args(); let verbose = notion.verbose; let exit_code = notion.run(&mut session).unwrap_or_else(|err| { - if verbose { - display_verbose_error(ErrorContext::Notion, &err); + let reporter = if verbose { + ErrorReporter::verbose() } else { - display_error(ErrorContext::Notion, &err); - } + ErrorReporter::new() + }; + reporter.report(ErrorContext::Notion, &err); session.add_event_error(ActivityKind::Notion, &err); err.exit_code() }); diff --git a/src/shim.rs b/src/shim.rs index 9286c8f0f..29d7ff95b 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -1,4 +1,4 @@ -use notion_core::error::{display_error, ErrorContext}; +use notion_core::error::{ErrorContext, ErrorReporter}; use notion_core::session::{ActivityKind, Session}; use notion_core::tool::execute_tool; @@ -21,7 +21,7 @@ pub fn main() { session.exit_tool(code); } Err(err) => { - display_error(ErrorContext::Shim, &err); + ErrorReporter::new().report(ErrorContext::Shim, &err); session.add_event_error(ActivityKind::Tool, &err); session.exit(ExitCode::ExecutionFailure); } From 9ea343102e3ec6f2addb19d6c3b5fbe2412412ca Mon Sep 17 00:00:00 2001 From: Charles Pierce Date: Sat, 23 Mar 2019 11:40:56 -0700 Subject: [PATCH 3/8] Write errors to logs --- Cargo.lock | 12 +++++++ crates/notion-core/Cargo.toml | 1 + crates/notion-core/src/error/reporter.rs | 43 ++++++++++++++++++++---- crates/notion-core/src/path/mod.rs | 4 +++ 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ac69e260..07bd253de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -249,6 +249,16 @@ dependencies = [ "either 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "chrono" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "num-integer 0.1.39 (registry+https://github.com/rust-lang/crates.io-index)", + "num-traits 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", + "time 0.1.42 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "clap" version = "2.32.0" @@ -1086,6 +1096,7 @@ version = "0.1.0" dependencies = [ "archive 0.1.0", "cfg-if 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", + "chrono 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "cmdline_words_parser 0.0.2 (registry+https://github.com/rust-lang/crates.io-index)", "console 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "detect-indent 0.1.0 (git+https://github.com/stefanpenner/detect-indent-rs)", @@ -2388,6 +2399,7 @@ dependencies = [ "checksum cc 1.0.29 (registry+https://github.com/rust-lang/crates.io-index)" = "4390a3b5f4f6bce9c1d0c00128379df433e53777fdd30e92f16a529332baec4e" "checksum cfg-if 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)" = "082bb9b28e00d3c9d39cc03e64ce4cea0f1bb9b3fde493f0cbc008472d22bdf4" "checksum chomp 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "9f74ad218e66339b11fd23f693fb8f1d621e80ba6ac218297be26073365d163d" +"checksum chrono 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)" = "45912881121cb26fad7c38c17ba7daa18764771836b34fab7d3fbd93ed633878" "checksum clap 2.32.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b957d88f4b6a63b9d70d5f454ac8011819c6efa7727858f458ab71c756ce2d3e" "checksum clicolors-control 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "1f84dec9bc083ce2503908cd305af98bd363da6f54bf8d4bf0ac14ee749ad5d1" "checksum clicolors-control 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "73abfd4c73d003a674ce5d2933fca6ce6c42480ea84a5ffe0a2dc39ed56300f9" diff --git a/crates/notion-core/Cargo.toml b/crates/notion-core/Cargo.toml index 817e82af2..f9cab28e2 100644 --- a/crates/notion-core/Cargo.toml +++ b/crates/notion-core/Cargo.toml @@ -37,3 +37,4 @@ regex = "1.0.6" dirs = "1.0.4" sha-1 = "0.8.1" hex = "0.3.2" +chrono = "0.4.6" diff --git a/crates/notion-core/src/error/reporter.rs b/crates/notion-core/src/error/reporter.rs index 747ad2db1..b79826c53 100644 --- a/crates/notion-core/src/error/reporter.rs +++ b/crates/notion-core/src/error/reporter.rs @@ -1,8 +1,12 @@ -use std::env; -use std::io::Result; -use std::path::PathBuf; +use std::env::{self, args_os}; +use std::fs::File; +use std::io::Write; +use crate::fs::ensure_containing_dir_exists; +use crate::path::log_dir; use crate::style::{format_error_details, format_error_message}; +use chrono::Local; +use failure::Error; use notion_fail::NotionError; const NOTION_DEV: &'static str = "NOTION_DEV"; @@ -50,12 +54,37 @@ impl ErrorReporter { } match write_error_log(message, details) { - Ok(_log_file) => {} - Err(_) => {} + Ok(log_file) => { + eprintln!("Error log written to: {}", log_file); + } + Err(_) => { + eprintln!("Unable to write error log!"); + } } } } -fn write_error_log(_message: String, _details: String) -> Result { - Ok(PathBuf::from("")) +fn write_error_log(message: String, details: String) -> Result { + let file_name = Local::now() + .format("notion-error-%Y-%m-%d_%H_%M_%S%.3f.log") + .to_string(); + let log_file_path = log_dir()?.join(&file_name); + + ensure_containing_dir_exists(&log_file_path)?; + let mut log_file = File::create(log_file_path)?; + + writeln!(log_file, "{}", collect_arguments())?; + writeln!(log_file, "Notion v{}", env!("CARGO_PKG_VERSION"))?; + writeln!(log_file)?; + write!(log_file, "{}{}", message, details)?; + + Ok(file_name) +} + +/// Combines all the arguments into a single String +fn collect_arguments() -> String { + args_os() + .map(|arg| arg.into_string().unwrap_or(String::from(""))) + .collect::>() + .join(" ") } diff --git a/crates/notion-core/src/path/mod.rs b/crates/notion-core/src/path/mod.rs index abd31f582..204e51342 100644 --- a/crates/notion-core/src/path/mod.rs +++ b/crates/notion-core/src/path/mod.rs @@ -40,6 +40,10 @@ pub fn tmp_dir() -> Fallible { Ok(notion_home()?.join("tmp")) } +pub fn log_dir() -> Fallible { + Ok(notion_home()?.join("log")) +} + pub fn node_inventory_dir() -> Fallible { Ok(inventory_dir()?.join("node")) } From b1227829da05fa0e3673a1299aeacc99d51fe17c Mon Sep 17 00:00:00 2001 From: Charles Pierce Date: Sat, 23 Mar 2019 17:40:27 -0700 Subject: [PATCH 4/8] Only write error log if there is an underlying error --- crates/notion-core/src/error/reporter.rs | 53 +++++++++++----- crates/notion-core/src/style.rs | 78 ++++++------------------ 2 files changed, 55 insertions(+), 76 deletions(-) diff --git a/crates/notion-core/src/error/reporter.rs b/crates/notion-core/src/error/reporter.rs index b79826c53..be863cf10 100644 --- a/crates/notion-core/src/error/reporter.rs +++ b/crates/notion-core/src/error/reporter.rs @@ -1,12 +1,13 @@ use std::env::{self, args_os}; +use std::fmt::Write as FmtWrite; use std::fs::File; -use std::io::Write; +use std::io::Write as IoWrite; use crate::fs::ensure_containing_dir_exists; use crate::path::log_dir; -use crate::style::{format_error_details, format_error_message}; +use crate::style::{format_error_cause, format_error_message}; use chrono::Local; -use failure::Error; +use failure::{Error, Fail}; use notion_fail::NotionError; const NOTION_DEV: &'static str = "NOTION_DEV"; @@ -20,6 +21,7 @@ pub enum ErrorContext { Shim, } +#[derive(PartialEq)] pub enum ErrorReporter { /// Reports errors in the standard, concise format Standard, @@ -44,26 +46,45 @@ impl ErrorReporter { } /// Report an error, both to the terminal and the error log - pub fn report(&self, cx: ErrorContext, err: &NotionError) { + pub fn report(self, cx: ErrorContext, err: &NotionError) { let message = format_error_message(cx, err); - let details = format_error_details(err); - match self { - ErrorReporter::Standard => eprint!("{}", message), - ErrorReporter::Verbose => eprint!("{}{}", message, details), - } + eprintln!("{}", message); + + // Only consider additional details if the error has an underlying cause + if let Some(inner) = err.as_fail().cause() { + let details = compose_error_details(err, inner); - match write_error_log(message, details) { - Ok(log_file) => { - eprintln!("Error log written to: {}", log_file); + if self == ErrorReporter::Verbose { + eprintln!(); + eprintln!("{}", details); } - Err(_) => { - eprintln!("Unable to write error log!"); + + match write_error_log(message, details) { + Ok(log_file) => { + eprintln!("Error details written to: {}", log_file); + } + Err(_) => { + eprintln!("Unable to write error log!"); + } } } } } +fn compose_error_details(err: &NotionError, inner: &Fail) -> String { + let mut details = format_error_cause(inner); + + // ISSUE #75 - Once we have a way to determine backtraces without RUST_BACKTRACE, we can make this always available + // Until then, we know that if the env var is not set, the backtrace will be empty + if env::var("RUST_BACKTRACE").is_ok() { + // Note: The implementation of `Display` for Backtrace includes a 'stack backtrace:' prefix + write!(details, "\n\n{}", err.backtrace()).expect("write! to a String doesn't fail"); + } + + details +} + fn write_error_log(message: String, details: String) -> Result { let file_name = Local::now() .format("notion-error-%Y-%m-%d_%H_%M_%S%.3f.log") @@ -76,7 +97,9 @@ fn write_error_log(message: String, details: String) -> Result { writeln!(log_file, "{}", collect_arguments())?; writeln!(log_file, "Notion v{}", env!("CARGO_PKG_VERSION"))?; writeln!(log_file)?; - write!(log_file, "{}{}", message, details)?; + writeln!(log_file, "{}", message)?; + writeln!(log_file)?; + writeln!(log_file, "{}", details)?; Ok(file_name) } diff --git a/crates/notion-core/src/style.rs b/crates/notion-core/src/style.rs index 9aa4dfd94..8534b8c60 100644 --- a/crates/notion-core/src/style.rs +++ b/crates/notion-core/src/style.rs @@ -1,11 +1,8 @@ //! The view layer of Notion, with utilities for styling command-line output. - -use std::env; -use std::fmt::Write; - use crate::error::ErrorContext; use archive::Origin; -use console::style; +use console::{style, StyledObject}; +use failure::Fail; use indicatif::{ProgressBar, ProgressStyle}; use notion_fail::NotionError; use term_size; @@ -21,85 +18,44 @@ Please feel free to reach out to us at \x1b[36m\x1b[1m@notionjs\x1b[0m on Twitte \x1b[1mhttps://github.com/notion-cli/notion/issues\x1b[0m "; -macro_rules! write_str { - ( $( $x:expr ),* ) => { - write!($( $x, )*).expect("write! with String cannot fail") - } -} - -macro_rules! writeln_str { - ( $( $x:expr ),* ) => { - writeln!($( $x, )*).expect("write! with String cannot fail") - } -} - -/// Formats the error message to a string +/// Format an error for output in the given context pub(crate) fn format_error_message(cx: ErrorContext, err: &NotionError) -> String { - let mut message = String::with_capacity(100); + let prefix = error_prefix(cx); - format_error_prefix(&mut message, cx); if err.is_user_friendly() { - writeln_str!(message, "{}", err); + format!("{} {}", prefix, err) } else { - writeln_str!(message, "{}", INTERNAL_ERROR_MESSAGE); + format!("{} {}", prefix, INTERNAL_ERROR_MESSAGE) } - - message } -/// Formats verbose error details to string -pub(crate) fn format_error_details(err: &NotionError) -> String { - let mut details = String::new(); - - format_error_cause(&mut details, err); - format_error_backtrace(&mut details, err); - - details +/// Format the underlying cause of an error +pub(crate) fn format_error_cause(inner: &Fail) -> String { + format!( + "{}{} {}", + style("cause").underlined().bold(), + style(":").bold(), + inner + ) } -/// Formats a styled prefix for an error -fn format_error_prefix(msg: &mut String, cx: ErrorContext) { +fn error_prefix(cx: ErrorContext) -> StyledObject<&'static str> { match cx { ErrorContext::Notion => { // Since the command here was `notion`, it would be redundant to say that this was // a Notion error, so we are less explicit in the heading. - write_str!(msg, "{} ", style("error:").red().bold()); + style("error:").red().bold() } ErrorContext::Shim => { // Since a Notion error is rare case for a shim, it can be surprising to a user. // To make it extra clear that this was a failure that happened in Notion when // attempting to delegate to a shim, we are more explicit about the fact that it's // a Notion error. - write_str!(msg, "{} ", style("Notion error:").red().bold()); + style("Notion error:").red().bold() } } } -/// Formats the underlying cause of an error, if it exists -fn format_error_cause(msg: &mut String, err: &NotionError) { - if let Some(inner) = err.as_fail().cause() { - writeln_str!(msg); - write_str!( - msg, - "{}{} ", - style("cause").bold().underlined(), - style(":").bold() - ); - writeln_str!(msg, "{}", inner); - } -} - -/// Formats the backtrace for an error, if available -fn format_error_backtrace(msg: &mut String, err: &NotionError) { - // ISSUE #75 - Once we have a way to determine backtraces without RUST_BACKTRACE, we can make this always available - // Until then, we know that if the env var is not set, the backtrace will be empty - if env::var("RUST_BACKTRACE").is_ok() { - writeln_str!(msg); - // Note: The implementation of `Display` for Backtrace includes a 'stack backtrace:' prefix - writeln_str!(msg, "{}", err.backtrace()); - } -} - /// Determines the string to display based on the Origin of the operation. fn action_str(origin: Origin) -> &'static str { match origin { From 8b58da5c4bd23c71af5799a3b1b941779f11b91a Mon Sep 17 00:00:00 2001 From: Chuck Pierce Date: Mon, 25 Mar 2019 08:54:03 -0700 Subject: [PATCH 5/8] Ensure notion-core has same version as root executable --- Cargo.lock | 4 ++-- crates/notion-core/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 07bd253de..3a2e7b031 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1077,7 +1077,7 @@ dependencies = [ "failure_derive 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "hamcrest2 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "mockito 0.14.1 (registry+https://github.com/rust-lang/crates.io-index)", - "notion-core 0.1.0", + "notion-core 0.2.2", "notion-fail 0.1.0", "notion-fail-derive 0.1.0", "rand 0.5.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1092,7 +1092,7 @@ dependencies = [ [[package]] name = "notion-core" -version = "0.1.0" +version = "0.2.2" dependencies = [ "archive 0.1.0", "cfg-if 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/crates/notion-core/Cargo.toml b/crates/notion-core/Cargo.toml index f9cab28e2..f212402ac 100644 --- a/crates/notion-core/Cargo.toml +++ b/crates/notion-core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "notion-core" -version = "0.1.0" +version = "0.2.2" authors = ["David Herman "] edition = "2018" From 55a7773b9d676dafe2c6fc9ceba3e5ee6d1b40e0 Mon Sep 17 00:00:00 2001 From: Chuck Pierce Date: Mon, 25 Mar 2019 09:45:02 -0700 Subject: [PATCH 6/8] Add acceptance tests for verbose errors --- tests/acceptance/main.rs | 1 + tests/acceptance/support/sandbox.rs | 7 +++ tests/acceptance/verbose_errors.rs | 83 +++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 tests/acceptance/verbose_errors.rs diff --git a/tests/acceptance/main.rs b/tests/acceptance/main.rs index 6ada957f3..29bb8b3fd 100644 --- a/tests/acceptance/main.rs +++ b/tests/acceptance/main.rs @@ -6,3 +6,4 @@ mod intercept_global_installs; mod notion_current; mod notion_deactivate; mod notion_pin; +mod verbose_errors; diff --git a/tests/acceptance/support/sandbox.rs b/tests/acceptance/support/sandbox.rs index 6df06aecd..745b65aaa 100644 --- a/tests/acceptance/support/sandbox.rs +++ b/tests/acceptance/support/sandbox.rs @@ -403,6 +403,9 @@ fn notion_tmp_dir() -> PathBuf { fn notion_bin_dir() -> PathBuf { notion_home().join("bin") } +fn notion_log_dir() -> PathBuf { + notion_home().join("log") +} fn notion_postscript() -> PathBuf { notion_tmp_dir().join("notion_tmp_1234.sh") } @@ -530,6 +533,10 @@ impl Sandbox { let postscript_file = notion_postscript(); read_file_to_string(postscript_file) } + + pub fn read_log_dir(&self) -> Option { + fs::read_dir(notion_log_dir()).ok() + } } impl Drop for Sandbox { diff --git a/tests/acceptance/verbose_errors.rs b/tests/acceptance/verbose_errors.rs new file mode 100644 index 000000000..1f350f20e --- /dev/null +++ b/tests/acceptance/verbose_errors.rs @@ -0,0 +1,83 @@ +use crate::support::sandbox::sandbox; +use hamcrest2::assert_that; +use hamcrest2::prelude::*; +use test_support::matchers::execs; + +use notion_fail::ExitCode; + +const NODE_VERSION_INFO: &'static str = r#"[ +{"version":"v10.99.1040","npm":"6.2.26","files":["linux-x64","osx-x64-tar","win-x64-zip","win-x86-zip"]}, +{"version":"v9.27.6","npm":"5.6.17","files":["linux-x64","osx-x64-tar","win-x64-zip","win-x86-zip"]}, +{"version":"v8.9.10","npm":"5.6.7","files":["linux-x64","osx-x64-tar","win-x64-zip","win-x86-zip"]}, +{"version":"v6.19.62","npm":"3.10.1066","files":["linux-x64","osx-x64-tar","win-x64-zip","win-x86-zip"]} +] +"#; + +#[test] +fn no_cause_shown_if_no_verbose_flag() { + let s = sandbox().node_available_versions(NODE_VERSION_INFO).build(); + + assert_that!( + s.notion("install node 10"), + execs() + .with_status(ExitCode::NetworkError as i32) + .with_stderr_does_not_contain("cause[..]") + ); +} + +#[test] +fn cause_shown_if_verbose_flag() { + let s = sandbox().node_available_versions(NODE_VERSION_INFO).build(); + + assert_that!( + s.notion("install node 10 --verbose"), + execs() + .with_status(ExitCode::NetworkError as i32) + .with_stderr_contains("cause[..]") + ); +} + +#[test] +fn no_cause_if_no_underlying_error() { + let s = sandbox().build(); + + assert_that!( + s.notion("use --verbose"), + execs() + .with_status(ExitCode::InvalidArguments as i32) + .with_stderr_does_not_contain("cause[..]") + ); +} + +#[test] +fn error_log_if_underlying_cause() { + let s = sandbox().node_available_versions(NODE_VERSION_INFO).build(); + + assert_that!( + s.notion("install node 10"), + execs() + .with_status(ExitCode::NetworkError as i32) + .with_stderr_contains("Error details written to[..]") + ); + + println!("ROOT DIRECTORY: {:?}", s.root()); + let mut log_dir_contents = s.read_log_dir().expect("Could not read log directory"); + assert_that!(log_dir_contents.next(), some()); +} + +#[test] +fn no_error_log_if_no_underlying_cause() { + let s = sandbox().build(); + + assert_that!( + s.notion("use"), + execs() + .with_status(ExitCode::InvalidArguments as i32) + .with_stderr_does_not_contain("Error details written to[..]") + ); + + // The log directory may not exist at all. If so, we know we didn't write to it + if let Some(mut log_dir_contents) = s.read_log_dir() { + assert_that!(log_dir_contents.next(), none()); + } +} From 70cc72b021f7d1ad72bc10de41f8dfa78d15ccf4 Mon Sep 17 00:00:00 2001 From: Chuck Pierce Date: Mon, 25 Mar 2019 10:31:04 -0700 Subject: [PATCH 7/8] Remove NOTION_DEV when testing verbose errors --- tests/acceptance/support/sandbox.rs | 12 ++++++++++++ tests/acceptance/verbose_errors.rs | 5 ++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/acceptance/support/sandbox.rs b/tests/acceptance/support/sandbox.rs index 745b65aaa..3257456b8 100644 --- a/tests/acceptance/support/sandbox.rs +++ b/tests/acceptance/support/sandbox.rs @@ -201,6 +201,7 @@ impl SandboxBuilder { root, mocks: vec![], env_vars: vec![], + env_vars_remove: vec![], path: OsString::new(), }, files: vec![], @@ -246,6 +247,12 @@ impl SandboxBuilder { self } + /// Unset an environment variable for the sandbox (chainable) + pub fn env_remove(mut self, name: &str) -> Self { + self.root.env_vars_remove.push(name.to_string()); + self + } + /// Add a directory to the PATH (chainable) pub fn path_dir(mut self, dir: &str) -> Self { self.path_dirs.push(PathBuf::from(dir)); @@ -453,6 +460,7 @@ pub struct Sandbox { root: PathBuf, mocks: Vec, env_vars: Vec, + env_vars_remove: Vec, path: OsString, } @@ -483,6 +491,10 @@ impl Sandbox { p.env(&env_var.name, &env_var.value); } + for env_var_name in &self.env_vars_remove { + p.env_remove(env_var_name); + } + p } diff --git a/tests/acceptance/verbose_errors.rs b/tests/acceptance/verbose_errors.rs index 1f350f20e..b304ce7f6 100644 --- a/tests/acceptance/verbose_errors.rs +++ b/tests/acceptance/verbose_errors.rs @@ -15,7 +15,10 @@ const NODE_VERSION_INFO: &'static str = r#"[ #[test] fn no_cause_shown_if_no_verbose_flag() { - let s = sandbox().node_available_versions(NODE_VERSION_INFO).build(); + let s = sandbox() + .env_remove("NOTION_DEV") + .node_available_versions(NODE_VERSION_INFO) + .build(); assert_that!( s.notion("install node 10"), From 1cf5ab4a6b7240c09840729272da115dddd27fcc Mon Sep 17 00:00:00 2001 From: Chuck Pierce Date: Mon, 25 Mar 2019 12:59:35 -0700 Subject: [PATCH 8/8] Refactor ErrorReporter constructor to clean up api --- Cargo.lock | 4 +- crates/notion-core/Cargo.toml | 2 +- crates/notion-core/src/error/reporter.rs | 79 +++++++++++++----------- src/main.rs | 8 +-- src/shim.rs | 2 +- 5 files changed, 49 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3a2e7b031..07bd253de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1077,7 +1077,7 @@ dependencies = [ "failure_derive 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "hamcrest2 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "mockito 0.14.1 (registry+https://github.com/rust-lang/crates.io-index)", - "notion-core 0.2.2", + "notion-core 0.1.0", "notion-fail 0.1.0", "notion-fail-derive 0.1.0", "rand 0.5.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1092,7 +1092,7 @@ dependencies = [ [[package]] name = "notion-core" -version = "0.2.2" +version = "0.1.0" dependencies = [ "archive 0.1.0", "cfg-if 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/crates/notion-core/Cargo.toml b/crates/notion-core/Cargo.toml index f212402ac..f9cab28e2 100644 --- a/crates/notion-core/Cargo.toml +++ b/crates/notion-core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "notion-core" -version = "0.2.2" +version = "0.1.0" authors = ["David Herman "] edition = "2018" diff --git a/crates/notion-core/src/error/reporter.rs b/crates/notion-core/src/error/reporter.rs index be863cf10..8b576cb1f 100644 --- a/crates/notion-core/src/error/reporter.rs +++ b/crates/notion-core/src/error/reporter.rs @@ -21,32 +21,38 @@ pub enum ErrorContext { Shim, } -#[derive(PartialEq)] -pub enum ErrorReporter { - /// Reports errors in the standard, concise format - Standard, +/// Reporter for showing errors to the terminal and error logs +pub struct ErrorReporter { + /// Notion version to display in error logs + version: String, - /// Reports errors with additional details - Verbose, + /// Flag indicating whether to report additional details to the terminal + verbose: bool, } impl ErrorReporter { - /// Create a new ErrorReporter of the default type - pub fn new() -> Self { - if env::var(NOTION_DEV).is_ok() { - ErrorReporter::Verbose + /// Create a new ErrorReporter from a verbose flag + pub fn from_flag(notion_version: &str, verbose: bool) -> Self { + if verbose { + ErrorReporter { + version: notion_version.to_string(), + verbose, + } } else { - ErrorReporter::Standard + ErrorReporter::from_env(notion_version) } } - /// Create a new verbose ErrorReporter - pub fn verbose() -> Self { - ErrorReporter::Verbose + /// Create a new ErrorReporter from the environment variables + pub fn from_env(notion_version: &str) -> Self { + ErrorReporter { + version: notion_version.to_string(), + verbose: env::var(NOTION_DEV).is_ok(), + } } /// Report an error, both to the terminal and the error log - pub fn report(self, cx: ErrorContext, err: &NotionError) { + pub fn report(&self, cx: ErrorContext, err: &NotionError) { let message = format_error_message(cx, err); eprintln!("{}", message); @@ -55,12 +61,12 @@ impl ErrorReporter { if let Some(inner) = err.as_fail().cause() { let details = compose_error_details(err, inner); - if self == ErrorReporter::Verbose { + if self.verbose { eprintln!(); eprintln!("{}", details); } - match write_error_log(message, details) { + match self.write_error_log(message, details) { Ok(log_file) => { eprintln!("Error details written to: {}", log_file); } @@ -70,6 +76,26 @@ impl ErrorReporter { } } } + + /// Write an error log with additional details about the error + fn write_error_log(&self, message: String, details: String) -> Result { + let file_name = Local::now() + .format("notion-error-%Y-%m-%d_%H_%M_%S%.3f.log") + .to_string(); + let log_file_path = log_dir()?.join(&file_name); + + ensure_containing_dir_exists(&log_file_path)?; + let mut log_file = File::create(log_file_path)?; + + writeln!(log_file, "{}", collect_arguments())?; + writeln!(log_file, "Notion v{}", self.version)?; + writeln!(log_file)?; + writeln!(log_file, "{}", message)?; + writeln!(log_file)?; + writeln!(log_file, "{}", details)?; + + Ok(file_name) + } } fn compose_error_details(err: &NotionError, inner: &Fail) -> String { @@ -85,25 +111,6 @@ fn compose_error_details(err: &NotionError, inner: &Fail) -> String { details } -fn write_error_log(message: String, details: String) -> Result { - let file_name = Local::now() - .format("notion-error-%Y-%m-%d_%H_%M_%S%.3f.log") - .to_string(); - let log_file_path = log_dir()?.join(&file_name); - - ensure_containing_dir_exists(&log_file_path)?; - let mut log_file = File::create(log_file_path)?; - - writeln!(log_file, "{}", collect_arguments())?; - writeln!(log_file, "Notion v{}", env!("CARGO_PKG_VERSION"))?; - writeln!(log_file)?; - writeln!(log_file, "{}", message)?; - writeln!(log_file)?; - writeln!(log_file, "{}", details)?; - - Ok(file_name) -} - /// Combines all the arguments into a single String fn collect_arguments() -> String { args_os() diff --git a/src/main.rs b/src/main.rs index cb9c01895..a0602fdb3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -16,12 +16,8 @@ pub fn main() { let notion = cli::Notion::from_args(); let verbose = notion.verbose; let exit_code = notion.run(&mut session).unwrap_or_else(|err| { - let reporter = if verbose { - ErrorReporter::verbose() - } else { - ErrorReporter::new() - }; - reporter.report(ErrorContext::Notion, &err); + ErrorReporter::from_flag(env!("CARGO_PKG_VERSION"), verbose) + .report(ErrorContext::Notion, &err); session.add_event_error(ActivityKind::Notion, &err); err.exit_code() }); diff --git a/src/shim.rs b/src/shim.rs index 29d7ff95b..76b235fc8 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -21,7 +21,7 @@ pub fn main() { session.exit_tool(code); } Err(err) => { - ErrorReporter::new().report(ErrorContext::Shim, &err); + ErrorReporter::from_env(env!("CARGO_PKG_VERSION")).report(ErrorContext::Shim, &err); session.add_event_error(ActivityKind::Tool, &err); session.exit(ExitCode::ExecutionFailure); }