Improve error handling

Create choose::Error and choose::Result
Improve error propogation
Improve error printing

Add tests for negative indices
Add get_negative_start_end tests
Fixup negative choice indexing

Remove most uses of unwrap

Fixes #38

src/choice/mod.rs

@@ -0,0 +1,267 @@
+use std::convert::TryInto;
+
+use crate::config::Config;
+use crate::error::Error;
+use crate::result::Result;
+use crate::writeable::Writeable;
+use crate::writer::WriteReceiver;
+
+#[cfg(test)]
+mod test;
+
+#[derive(Debug)]
+pub struct Choice {
+    pub start: isize,
+    pub end: isize,
+    pub kind: ChoiceKind,
+    negative_index: bool,
+    reversed: bool,
+}
+
+#[derive(Debug, PartialEq)]
+pub enum ChoiceKind {
+    Single,
+    RustExclusiveRange,
+    RustInclusiveRange,
+    ColonRange,
+}
+
+impl Choice {
+    pub fn new(start: isize, end: isize, kind: ChoiceKind) -> Self {
+        let negative_index = start < 0 || end < 0;
+        let reversed = end < start && !(start >= 0 && end < 0);
+        Choice {
+            start,
+            end,
+            kind,
+            negative_index,
+            reversed,
+        }
+    }
+
+    pub fn print_choice<W: WriteReceiver>(
+        &self,
+        line: &str,
+        config: &Config,
+        handle: &mut W,
+    ) -> Result<()> {
+        if config.opt.character_wise {
+            self.print_choice_generic(line.chars(), config, handle)
+        } else {
+            let line_iter = config
+                .separator
+                .split(line)
+                .filter(|s| !s.is_empty() || config.opt.non_greedy);
+            self.print_choice_generic(line_iter, config, handle)
+        }
+    }
+
+    pub fn is_reverse_range(&self) -> bool {
+        self.reversed
+    }
+
+    pub fn has_negative_index(&self) -> bool {
+        self.negative_index
+    }
+
+    fn print_choice_generic<W, T, I>(
+        &self,
+        mut iter: I,
+        config: &Config,
+        handle: &mut W,
+    ) -> Result<()>
+    where
+        W: WriteReceiver,
+        T: Writeable,
+        I: Iterator<Item = T>,
+    {
+        if self.is_reverse_range() && !self.has_negative_index() {
+            self.print_choice_reverse(iter, config, handle)?;
+        } else if self.has_negative_index() {
+            self.print_choice_negative(iter, config, handle)?;
+        } else {
+            if self.start > 0 {
+                iter.nth((self.start - 1).try_into()?);
+            }
+            let range = self
+                .end
+                .checked_sub(self.start)
+                .ok_or_else(|| Error::Config("expected end > start".into()))?;
+            Choice::print_choice_loop_max_items(iter, config, handle, range)?;
+        }
+
+        Ok(())
+    }
+
+    fn print_choice_loop_max_items<W, T, I>(
+        iter: I,
+        config: &Config,
+        handle: &mut W,
+        max_items: isize,
+    ) -> Result<()>
+    where
+        W: WriteReceiver,
+        T: Writeable,
+        I: Iterator<Item = T>,
+    {
+        let mut peek_iter = iter.peekable();
+        for i in 0..=max_items {
+            match peek_iter.next() {
+                Some(s) => {
+                    handle.write_choice(s, config, peek_iter.peek().is_some() && i != max_items)?;
+                }
+                None => break,
+            };
+        }
+
+        Ok(())
+    }
+
+    /// Print choices that include at least one negative index
+    fn print_choice_negative<W, T, I>(&self, iter: I, config: &Config, handle: &mut W) -> Result<()>
+    where
+        W: WriteReceiver,
+        T: Writeable,
+        I: Iterator<Item = T>,
+    {
+        let vec = iter.collect::<Vec<_>>();
+
+        if let Some((start, end)) = self.get_negative_start_end(&vec)? {
+            if end > start {
+                for word in vec[start..std::cmp::min(end, vec.len() - 1)].iter() {
+                    handle.write_choice(*word, config, true)?;
+                }
+                handle.write_choice(vec[std::cmp::min(end, vec.len() - 1)], config, false)?;
+            } else if self.start < 0 {
+                for word in vec[end + 1..=std::cmp::min(start, vec.len() - 1)]
+                    .iter()
+                    .rev()
+                {
+                    handle.write_choice(*word, config, true)?;
+                }
+                handle.write_choice(vec[end], config, false)?;
+            }
+        }
+
+        Ok(())
+    }
+
+    fn print_choice_reverse<W, T, I>(
+        &self,
+        mut iter: I,
+        config: &Config,
+        handle: &mut W,
+    ) -> Result<()>
+    where
+        W: WriteReceiver,
+        T: Writeable,
+        I: Iterator<Item = T>,
+    {
+        if self.end > 0 {
+            iter.nth((self.end - 1).try_into()?);
+        }
+
+        let mut stack = Vec::new();
+        for i in 0..=(self.start - self.end) {
+            match iter.next() {
+                Some(s) => stack.push(s),
+                None => break,
+            }
+
+            if self.start <= self.end + i {
+                break;
+            }
+        }
+
+        let mut peek_iter = stack.iter().rev().peekable();
+        while let Some(s) = peek_iter.next() {
+            handle.write_choice(*s, config, peek_iter.peek().is_some())?;
+        }
+
+        Ok(())
+    }
+
+    /// Get the absolute indexes of a choice range based on the slice length
+    ///
+    /// N.B. that this assumes that at least one index is negative - do not try to call this
+    /// function with a purely positive range.
+    ///
+    /// Returns Ok(None) if the resulting choice range would not include any item in the slice.
+    fn get_negative_start_end<T>(&self, slice: &[T]) -> Result<Option<(usize, usize)>> {
+        if slice.len() == 0 {
+            return Ok(None);
+        }
+
+        let start_abs = self.start.checked_abs().ok_or_else(|| {
+            Error::Config(format!(
+                "Minimum index value supported is isize::MIN + 1 ({})",
+                isize::MIN + 1
+            ))
+        })?;
+
+        let slice_len_as_isize = slice.len().try_into()?;
+
+        if self.kind == ChoiceKind::Single {
+            if start_abs <= slice_len_as_isize {
+                let idx = (slice_len_as_isize - start_abs).try_into()?;
+                Ok(Some((idx, idx)))
+            } else {
+                Ok(None)
+            }
+        } else {
+            let end_abs = self.end.checked_abs().ok_or_else(|| {
+                Error::Config(format!(
+                    "Minimum index value supported is isize::MIN + 1 ({})",
+                    isize::MIN + 1
+                ))
+            })?;
+
+            if self.start >= 0 {
+                // then we assume self.end is negative
+                let start = self.start.try_into()?;
+
+                if end_abs <= slice_len_as_isize
+                    || start <= slice.len()
+                    || (end_abs > slice_len_as_isize && start > slice.len())
+                {
+                    let end = slice.len().saturating_sub(end_abs.try_into()?);
+                    Ok(Some((
+                        std::cmp::min(start, slice.len().saturating_sub(1)),
+                        std::cmp::min(end, slice.len().saturating_sub(1)),
+                    )))
+                } else {
+                    Ok(None)
+                }
+            } else if self.end >= 0 {
+                // then we assume self.start is negative
+                let end = self.end.try_into()?;
+
+                if start_abs <= slice_len_as_isize
+                    || end <= slice.len()
+                    || (start_abs > slice_len_as_isize && end > slice.len())
+                {
+                    let start = slice.len().saturating_sub(start_abs.try_into()?);
+                    Ok(Some((
+                        std::cmp::min(start, slice.len().saturating_sub(1)),
+                        std::cmp::min(end, slice.len().saturating_sub(1)),
+                    )))
+                } else {
+                    Ok(None)
+                }
+            } else {
+                // both indices are negative
+                let start = slice.len().saturating_sub(start_abs.try_into()?);
+                let end = slice.len().saturating_sub(end_abs.try_into()?);
+
+                if start_abs <= slice_len_as_isize || end_abs <= slice_len_as_isize {
+                    Ok(Some((
+                        std::cmp::min(start, slice.len().saturating_sub(1)),
+                        std::cmp::min(end, slice.len().saturating_sub(1)),
+                    )))
+                } else {
+                    Ok(None)
+                }
+            }
+        }
+    }
+}

src/choice/test/get_negative_start_end.rs

@@ -0,0 +1,183 @@
+use super::*;
+use crate::Error;
+
+#[test]
+fn positive_negative_1() {
+    let config = Config::from_iter(vec!["choose", "2:-1"]);
+    let slice = &[1, 2, 3, 4, 5];
+    assert_eq!(
+        Some((2, 4)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn positive_negative_gt1() {
+    let config = Config::from_iter(vec!["choose", "1:-3"]);
+    let slice = &[1, 2, 3, 4, 5];
+    assert_eq!(
+        Some((1, 2)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn negative_positive() {
+    let config = Config::from_iter(vec!["choose", "-3:4"]);
+    let slice = &[1, 2, 3, 4, 5];
+    assert_eq!(
+        Some((2, 4)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn negative_negative() {
+    let config = Config::from_iter(vec!["choose", "-3:-4"]);
+    let slice = &[1, 2, 3, 4, 5];
+    assert_eq!(
+        Some((2, 1)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn negative1_negative1() {
+    let config = Config::from_iter(vec!["choose", "-1:-1"]);
+    let slice = &[1, 2, 3, 4, 5];
+    assert_eq!(
+        Some((4, 4)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn negative_nonexisting_positive() {
+    let config = Config::from_iter(vec!["choose", "-3:9"]);
+    let slice = &[1, 2, 3, 4, 5];
+    assert_eq!(
+        Some((2, 4)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn negative_negative_on_empty() {
+    let config = Config::from_iter(vec!["choose", "-3:-1"]);
+    let slice = &[0u8; 0];
+    assert_eq!(
+        None,
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    )
+}
+
+#[test]
+fn negative_positive_on_empty() {
+    let config = Config::from_iter(vec!["choose", "-3:5"]);
+    let slice = &[0u8; 0];
+    assert_eq!(
+        None,
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    )
+}
+
+#[test]
+fn positive_negative_on_empty() {
+    let config = Config::from_iter(vec!["choose", "2:-1"]);
+    let slice = &[0u8; 0];
+    assert_eq!(
+        None,
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    )
+}
+
+#[test]
+fn negative_positive_all() {
+    let config = Config::from_iter(vec!["choose", "-5:9"]);
+    let slice = &[0, 1, 2, 3];
+    assert_eq!(
+        Some((0, 3)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn negative_positive_some() {
+    let config = Config::from_iter(vec!["choose", "-5:2"]);
+    let slice = &[0, 1, 2, 3];
+    assert_eq!(
+        Some((0, 2)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn positive_negative_all() {
+    let config = Config::from_iter(vec!["choose", "9:-5"]);
+    let slice = &[0, 1, 2, 3];
+    assert_eq!(
+        Some((3, 0)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn positive_negative_some() {
+    let config = Config::from_iter(vec!["choose", "9:-2"]);
+    let slice = &[0, 1, 2, 3];
+    assert_eq!(
+        Some((3, 2)),
+        config.opt.choices[0].get_negative_start_end(slice).unwrap()
+    );
+}
+
+#[test]
+fn error_when_choice_is_isize_min() {
+    let isize_min = format!("{}", isize::MIN);
+    let config = Config::from_iter(vec!["choose", &isize_min]);
+    let slice = &[0, 1, 2, 3];
+
+    let err = config.opt.choices[0]
+        .get_negative_start_end(slice)
+        .unwrap_err();
+
+    if let Error::Config(s) = err {
+        assert!(s.contains("Minimum index value supported is isize::MIN"));
+    } else {
+        panic!("Expected Error::Config, found {}", err)
+    }
+}
+
+#[test]
+fn error_when_choice_start_is_isize_min() {
+    let choice = format!("{}:4", isize::MIN);
+    let config = Config::from_iter(vec!["choose", &choice]);
+    let slice = &[0, 1, 2, 3];
+
+    let err = config.opt.choices[0]
+        .get_negative_start_end(slice)
+        .unwrap_err();
+
+    if let Error::Config(s) = err {
+        assert!(s.contains("Minimum index value supported is isize::MIN"));
+    } else {
+        panic!("Expected Error::Config, found {}", err)
+    }
+}
+
+#[test]
+fn error_when_choice_end_is_isize_min() {
+    let choice = format!("4:{}", isize::MIN);
+    let config = Config::from_iter(vec!["choose", &choice]);
+    let slice = &[0, 1, 2, 3];
+
+    let err = config.opt.choices[0]
+        .get_negative_start_end(slice)
+        .unwrap_err();
+
+    if let Error::Config(s) = err {
+        assert!(s.contains("Minimum index value supported is isize::MIN"));
+    } else {
+        panic!("Expected Error::Config, found {}", err)
+    }
+}

src/choice/test/is_reverse_range.rs

@@ -0,0 +1,31 @@
+use super::*;
+
+#[test]
+fn is_field_reversed() {
+    let config = Config::from_iter(vec!["choose", "0"]);
+    assert_eq!(false, config.opt.choices[0].is_reverse_range());
+}
+
+#[test]
+fn is_field_range_no_start_reversed() {
+    let config = Config::from_iter(vec!["choose", ":2"]);
+    assert_eq!(false, config.opt.choices[0].is_reverse_range());
+}
+
+#[test]
+fn is_field_range_no_end_reversed() {
+    let config = Config::from_iter(vec!["choose", "2:"]);
+    assert_eq!(false, config.opt.choices[0].is_reverse_range());
+}
+
+#[test]
+fn is_field_range_no_start_or_end_reversed() {
+    let config = Config::from_iter(vec!["choose", ":"]);
+    assert_eq!(false, config.opt.choices[0].is_reverse_range());
+}
+
+#[test]
+fn is_reversed_field_range_reversed() {
+    let config = Config::from_iter(vec!["choose", "4:2"]);
+    assert_eq!(true, config.opt.choices[0].is_reverse_range());
+}

src/choice/test/mod.rs

@@ -0,0 +1,55 @@
+use crate::config::Config;
+use crate::opt::Opt;
+use std::ffi::OsString;
+use std::io::{self, BufWriter, Write};
+use structopt::StructOpt;
+
+mod get_negative_start_end;
+mod is_reverse_range;
+mod print_choice;
+
+impl Config {
+    pub fn from_iter<I>(iter: I) -> Self
+    where
+        I: IntoIterator,
+        I::Item: Into<OsString> + Clone,
+    {
+        Config::new(Opt::from_iter(iter))
+    }
+}
+
+struct MockStdout {
+    pub buffer: String,
+}
+
+impl MockStdout {
+    fn new() -> Self {
+        MockStdout {
+            buffer: String::new(),
+        }
+    }
+
+    fn str_from_buf_writer(b: BufWriter<MockStdout>) -> String {
+        match b.into_inner() {
+            Ok(b) => b.buffer,
+            Err(_) => panic!("Failed to access BufWriter inner writer"),
+        }
+        .trim_end()
+        .to_string()
+    }
+}
+
+impl Write for MockStdout {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        let mut bytes_written = 0;
+        for i in buf {
+            self.buffer.push(*i as char);
+            bytes_written += 1;
+        }
+        Ok(bytes_written)
+    }
+
+    fn flush(&mut self) -> io::Result<()> {
+        Ok(())
+    }
+}

src/error.rs

@@ -0,0 +1,63 @@
+use std::error::Error as StdError;
+use std::fmt;
+use std::num::TryFromIntError;
+
+#[derive(Debug)]
+pub enum Error {
+    Io(std::io::Error),
+    ParseRange(ParseRangeError),
+    TryFromInt(TryFromIntError),
+    Config(String),
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::Io(io) => write!(f, "{}", io),
+            Self::ParseRange(pr) => write!(f, "{}", pr),
+            Self::TryFromInt(tfi) => write!(f, "{}", tfi),
+            Self::Config(c) => write!(f, "{}", c),
+        }
+    }
+}
+
+impl StdError for Error {}
+
+impl From<std::io::Error> for Error {
+    fn from(e: std::io::Error) -> Self {
+        Self::Io(e)
+    }
+}
+
+impl From<ParseRangeError> for Error {
+    fn from(e: ParseRangeError) -> Self {
+        Self::ParseRange(e)
+    }
+}
+
+impl From<TryFromIntError> for Error {
+    fn from(e: TryFromIntError) -> Self {
+        Self::TryFromInt(e)
+    }
+}
+
+#[derive(Debug)]
+pub struct ParseRangeError {
+    source_str: String,
+}
+
+impl ParseRangeError {
+    pub fn new(source_str: &str) -> Self {
+        ParseRangeError {
+            source_str: String::from(source_str),
+        }
+    }
+}
+
+impl fmt::Display for ParseRangeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.source_str)
+    }
+}
+
+impl StdError for ParseRangeError {}

src/errors.rs

@@ -1,23 +0,0 @@
-use std::error::Error;
-use std::fmt;
-
-#[derive(Debug)]
-pub struct ParseRangeError {
-    source_str: String,
-}
-
-impl ParseRangeError {
-    pub fn new(source_str: &str) -> Self {
-        ParseRangeError {
-            source_str: String::from(source_str),
-        }
-    }
-}
-
-impl fmt::Display for ParseRangeError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}", self.source_str)
-    }
-}
-
-impl Error for ParseRangeError {}

src/main.rs

@@ -8,16 +8,20 @@ extern crate lazy_static;
 
 mod choice;
 mod config;
-mod errors;
+mod error;
 mod escape;
 mod opt;
 mod parse;
 mod parse_error;
 mod reader;
+mod result;
 mod writeable;
 mod writer;
+
 use config::Config;
+use error::Error;
 use opt::Opt;
+use result::Result;
 use writer::WriteReceiver;
 
 fn main() {
@@ -32,18 +36,23 @@ fn main() {
 
     match exit_result {
         Ok(_) => (),
-        Err(e) => {
-            if e.kind() == io::ErrorKind::BrokenPipe {
-                // BrokenPipe means whoever is reading the output hung up, we should
-                // gracefully exit
-            } else {
-                eprintln!("Failed to write to output: {}", e)
+        Err(err) => {
+            match err {
+                Error::Io(e) => {
+                    if e.kind() == io::ErrorKind::BrokenPipe {
+                        // BrokenPipe means whoever is reading the output hung up, we should
+                        // gracefully exit
+                    } else {
+                        eprintln!("Failed to write to output: {}", e)
+                    }
+                }
+                e @ _ => eprintln!("Error: {}", e),
             }
         }
     }
 }
 
-fn main_generic<W: WriteReceiver>(opt: Opt, handle: &mut W) -> io::Result<()> {
+fn main_generic<W: WriteReceiver>(opt: Opt, handle: &mut W) -> Result<()> {
     let config = Config::new(opt);
 
     let read = match &config.opt.input {
@@ -65,7 +74,7 @@ fn main_generic<W: WriteReceiver>(opt: Opt, handle: &mut W) -> io::Result<()> {
         match line {
             Ok(l) => {
                 let l = if config.opt.character_wise || config.opt.field_separator.is_some() {
-                    &l[0..l.len() - 1]
+                    &l[0..l.len().saturating_sub(1)]
                 } else {
                     &l
                 };

src/parse.rs

@@ -2,7 +2,7 @@ use backslash::escape_ascii;
 use regex::Regex;
 
 use crate::choice::{Choice, ChoiceKind};
-use crate::errors::ParseRangeError;
+use crate::error::ParseRangeError;
 use crate::parse_error::ParseError;
 
 lazy_static! {

src/parse_error.rs

@@ -1,7 +1,7 @@
 #[derive(Debug)]
 pub enum ParseError {
     ParseIntError(std::num::ParseIntError),
-    ParseRangeError(crate::errors::ParseRangeError),
+    ParseRangeError(crate::error::ParseRangeError),
 }
 
 impl ToString for ParseError {

src/result.rs

@@ -0,0 +1,3 @@
+use crate::error::Error;
+
+pub type Result<T> = std::result::Result<T, Error>;

test/alphabet.txt

@@ -0,0 +1,19 @@
+a
+b c
+d e f
+
+g h i
+j k
+l
+
+m
+n o
+p q r
+
+s t u
+v w
+x
+
+y
+
+z

test/choose_-1.txt

@@ -0,0 +1,19 @@
+a
+c
+f
+
+i
+k
+l
+
+m
+o
+r
+
+u
+w
+x
+
+y
+
+z

test/choose_-2.txt

@@ -0,0 +1,19 @@
+
+b
+e
+
+h
+j
+
+
+
+n
+q
+
+t
+v
+
+
+
+
+

test/e2e_test.sh

@@ -19,6 +19,8 @@ diff -w <(cargo run -- 1 3 -o % -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${t
 diff -w <(cargo run -- 1 3 -o '' -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_1_3of.txt")
 diff -w <(cargo run -- 3:6 -c -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_0_3_c.txt")
 diff -w <(cargo run -- 2 -x -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_2_x.txt")
+diff -w <(cargo run -- -1 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_-1.txt")
+diff -w <(cargo run -- -2 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_-2.txt")
 # add tests for different delimiters
 # add tests using piping