diff --git a/src/print.rs b/src/print.rs index db47882..576d9f9 100644 --- a/src/print.rs +++ b/src/print.rs @@ -1,19 +1,91 @@ use std::fmt; +use std::io; +use std::io::Write; +use std::result; use termion::color::{Color, Fg}; +use thiserror::Error; +pub(crate) type Result = result::Result<(), Error>; + +/// Error is a simple wrapper for `io::Error` that differentiates between certain error kinds as part of the type. +/// +/// In general, this type exists because of ; println! panics on +/// broken pipe errors. Though we could just ignore the errors, we need some way to differentiate between it and other +/// errors. This could be done with `io::Error::kind`, but this wrapper makes it explicit it should be handled with an +/// action such as terminating gracefully. It's silly and annoying, but it's how it is. +#[derive(Error, Debug)] +pub(crate) enum Error { + #[error("{0}")] + BrokenPipe(io::Error), + #[error("{0}")] + Other(io::Error), +} + +impl From for Error { + fn from(err: io::Error) -> Self { + match err.kind() { + io::ErrorKind::BrokenPipe => Self::BrokenPipe(err), + _ => Self::Other(err), + } + } +} + +/// Printer represents an object that can perform some kind of printing, such as by the print! macro pub(crate) trait Printer { - fn print(&self, msg: S); + /// Print the given message. + /// + /// # Errors + /// In the event of any i/o error, an error is returned. The type [Error] gives implementors the freendom to specify + /// whether or not this error was due to some kind of broken pipe error, which callers may choose to execute specifc + /// behavior. The docs of [Error] specify more information about this. + fn print(&self, msg: S) -> Result; - fn colored_print(&self, color: Fg, msg: S) { + /// Print the given message with the given foreground color. + /// + /// # Errors + /// In the event of any i/o error, an error is returned. The type [Error] gives implementors the freendom to specify + /// whether or not this error was due to some kind of broken pipe error, which callers may choose to execute specifc + /// behavior. The docs of [Error] specify more information about this. + fn colored_print(&self, color: Fg, msg: S) -> Result { let colored_message = format!("{}{}", color, msg); - self.print(colored_message); + + self.print(colored_message) } } pub(crate) struct StdoutPrinter; impl Printer for StdoutPrinter { - fn print(&self, msg: S) { - print!("{}", msg); + fn print(&self, msg: S) -> Result { + let mut stdout = io::stdout(); + match write!(stdout, "{}", msg) { + Err(err) => Err(Error::from(err)), + Ok(_) => Ok(()), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use test_case::test_case; + + #[test_case( + io::Error::new(io::ErrorKind::BrokenPipe, "broken pipe"), + &|err| matches!(err, Error::BrokenPipe(_)); + "BrokenPipe results in BrokenPipe variant" + )] + #[test_case( + io::Error::new(io::ErrorKind::Interrupted, "can't print, we're busy"), + &|err| matches!(err, Error::Other(_)); + "non-BrokenPipe produces Other variant" + )] + fn test_error_from_io_err(from: io::Error, matches: &dyn Fn(&Error) -> bool) { + let produced_err = Error::from(from); + assert!( + matches(&produced_err), + "enum did not match: got {:?}", + &produced_err + ); } } diff --git a/src/sink.rs b/src/sink.rs index acbb5c5..85a9abc 100644 --- a/src/sink.rs +++ b/src/sink.rs @@ -1,6 +1,8 @@ +use crate::print; use crate::print::{Printer, StdoutPrinter}; use grep::searcher::{Searcher, Sink, SinkContext, SinkMatch}; use std::error; +use std::io; use termion::color; use thiserror::Error; @@ -12,6 +14,32 @@ pub(crate) struct ContextPrintingSink { enum Error { #[error("The given searcher does not have passthru enabled. This is required for proper functionality")] NoPassthruOnSearcher, + #[error("Print failure: {0}")] + PrintFailed(io::Error), +} + +impl From for Error { + fn from(err: print::Error) -> Self { + let io_err = match err { + print::Error::BrokenPipe(wrapped) | print::Error::Other(wrapped) => wrapped, + }; + + Error::PrintFailed(io_err) + } +} + +impl ContextPrintingSink

{ + // TODO: this box should really just return an Error. + fn get_sink_result_for_print_result(res: print::Result) -> Result> { + match res { + // TODO: get rid of this boxing + Err(print::Error::Other(_)) => Err(Box::new(Error::from(res.unwrap_err()))), + // It is not an error case to have a broken pipe; it just means we can't output anything more and we + // shouldn't keep searching + Err(print::Error::BrokenPipe(_)) => Ok(false), + Ok(_) => Ok(true), + } + } } impl ContextPrintingSink { @@ -49,12 +77,12 @@ impl Sink for ContextPrintingSink

{ ) -> Result { Self::validate_searcher(searcher)?; - self.printer.colored_print( + let print_res = self.printer.colored_print( color::Fg(color::Red), std::str::from_utf8(sink_match.bytes()).unwrap(), ); - Ok(true) + Self::get_sink_result_for_print_result(print_res) } fn context( @@ -65,9 +93,9 @@ impl Sink for ContextPrintingSink

{ Self::validate_searcher(searcher)?; let data = std::str::from_utf8(context.bytes()).unwrap(); - self.printer.print(data); + let print_res = self.printer.print(data); - Ok(true) + Self::get_sink_result_for_print_result(print_res) } } @@ -78,23 +106,47 @@ mod tests { use grep::searcher::SearcherBuilder; use std::cell::RefCell; use std::fmt; + use std::io; use test_case::test_case; #[derive(Default)] struct MockPrinter { messages: RefCell>, colored_messages: RefCell>, + next_error: RefCell>, + } + + impl MockPrinter { + fn fail_next(&mut self, error: print::Error) { + self.next_error.replace(Some(error)); + } } impl Printer for &MockPrinter { - fn print(&self, msg: S) { + fn print(&self, msg: S) -> print::Result { self.messages.borrow_mut().push(msg.to_string()); + + if self.next_error.borrow().is_some() { + Err(self.next_error.replace(None).unwrap()) + } else { + Ok(()) + } } - fn colored_print(&self, _color: color::Fg, msg: S) { + fn colored_print( + &self, + _color: color::Fg, + msg: S, + ) -> print::Result { // Unfortunately, termion colors don't implement PartialEq, so checking for the exact color is not // feasible unless we wanted to write a wrapper, which I don't care enough to just for unit testing self.colored_messages.borrow_mut().push(msg.to_string()); + + if self.next_error.borrow().is_some() { + Err(self.next_error.replace(None).unwrap()) + } else { + Ok(()) + } } } @@ -126,11 +178,11 @@ mod tests { printer: &mock_printer, }; - let res = SearcherBuilder::new() - .line_number(true) - .passthru(true) - .build() - .search_slice(matcher, LIPSUM.as_bytes(), sink); + let res = SearcherBuilder::new().passthru(true).build().search_slice( + matcher, + LIPSUM.as_bytes(), + sink, + ); if let Err(err) = res { panic!("failed to search: {}", err) } @@ -163,6 +215,39 @@ mod tests { ); } + #[test_case(".", 0, 1; "failure on first match will only attempt to print that match")] + #[test_case("this doesn't appear in lorem ipsum", 1, 0; "never matching will only attempt to print the first line")] + fn test_does_not_attempt_to_print_after_broken_pipe_error( + pattern: &str, + num_uncolored_messages: usize, + num_colored_messages: usize, + ) { + let matcher = RegexMatcher::new(pattern).expect("regexp doesn't compile"); + + let mut mock_printer = MockPrinter::default(); + + let broken_pipe_err = + print::Error::from(io::Error::new(io::ErrorKind::BrokenPipe, "broken pipe")); + mock_printer.fail_next(broken_pipe_err); + + let sink = ContextPrintingSink { + printer: &mock_printer, + }; + + let res = SearcherBuilder::new().passthru(true).build().search_slice( + matcher, + LIPSUM.as_bytes(), + sink, + ); + + assert!(!res.is_err(), "failed to search: {:?}", res.unwrap_err()); + assert_eq!( + num_colored_messages, + mock_printer.colored_messages.borrow().len() + ); + assert_eq!(num_uncolored_messages, mock_printer.messages.borrow().len()); + } + // TODO: This is a bit overkill for a single setting, and could probably be simplified enum RequiredSearcherSettings { Passthru,