Better error handling

Cargo.toml

@@ -12,4 +12,5 @@ edition = "2018"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-regex = "1.3.7"
+regex = "1.3.7"
+failure = "0.1.8"

src/actions/mod.rs

@@ -10,6 +10,7 @@ use std::fmt;
 use std::fs::File;
 use std::io::BufRead;
 use std::io::BufReader;
+use failure::Error;
 
 #[derive(Debug, Default)]
 pub struct Dir {
@@ -55,93 +56,144 @@ enum ActionKind {
     Driver,
     License,
     Link,
+    Legacy,
+    Unknown{action: String},
 }
 
-#[derive(Debug)]
+//TODO Multierror and no failure for these cases
+#[derive(Debug, Fail)]
 pub enum ManifestError {
-    EmptyVec,
+    #[fail(display = "unknown action {} at line {}", action, line)]
-    // We will defer to the parse error implementation for their error.
+    UnknownAction {
-    // Supplying extra info requires adding more data to the type.
+        line: usize,
-    Read(std::io::Error),
+        action: String,
-    Regex(regex::Error),
+    },
 }
 
-impl fmt::Display for ManifestError {
+pub fn parse_manifest_file(filename: String) -> Result<Manifest, Error> {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    let mut m = Manifest::new();
-        match *self {
+    let f = File::open(filename)?;
-            ManifestError::EmptyVec => write!(f, "please use a vector with at least one element"),
-            // This is a wrapper, so defer to the underlying types' implementation of `fmt`.
-            ManifestError::Read(ref e) => e.fmt(f),
-            ManifestError::Regex(ref e) => e.fmt(f),
-        }
-    }
-}
 
-impl error::Error for ManifestError {
-    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
-        match *self {
-            ManifestError::EmptyVec => None,
-            // The cause is the underlying implementation error type. Is implicitly
-            // cast to the trait object `&error::Error`. This works because the
-            // underlying type already implements the `Error` trait.
-            ManifestError::Read(ref e) => Some(e),
-            ManifestError::Regex(ref e) => Some(e),
-        }
-    }
-}
-
-pub fn parse_manifest_file(filename: String) -> Result<Manifest, ManifestError> {
-    let mut manifest = Manifest::new();
-    let f = match File::open(filename) {
-        Ok(file) => file,
-        Err(e) => return Err(ManifestError::Read(e)),
-    };
     let file = BufReader::new(&f);
-    for line_read in file.lines() {
+
-        let line = match line_read {
+    for (line_nr, line_read) in file.lines().enumerate() {
-            Ok(l) => l,
+        let line = line_read?;
-            Err(e) => return Err(ManifestError::Read(e)),
+        match determine_action_kind(&line) {
-        };
+            ActionKind::Attr => {
-        if is_attr_action(&line) {
+                let attr = parse_attr_action(String::from(line))?;
-            match parse_attr_action(line) {
+                m.attributes.push(attr)
-                Ok(attr) => manifest.attributes.push(attr),
+            }
-                Err(e) => return Err(e),
+            ActionKind::Dir => {
+
+            }
+            ActionKind::File => {
+
+            }
+            ActionKind::Dependency => {
+
+            }
+            ActionKind::User => {
+
+            }
+            ActionKind::Group => {
+
+            }
+            ActionKind::Driver => {
+
+            }
+            ActionKind::License => {
+
+            }
+            ActionKind::Link => {
+
+            }
+            ActionKind::Legacy => {
+
+            }
+            ActionKind::Unknown{action} => {
+                Err(ManifestError::UnknownAction {action, line: line_nr})?;
             }
         }
     }
-    return Ok(manifest);
+
+    return Ok(m);
 }
 
-pub fn parse_manifest_string(manifest: String) -> Result<Manifest, ManifestError> {
+pub fn parse_manifest_string(manifest: String) -> Result<Manifest, Error> {
     let mut m = Manifest::new();
-    for line in manifest.lines() {
+    for (line_nr, line) in manifest.lines().enumerate() {
-        if is_attr_action(&String::from(line)) {
+        match determine_action_kind(&line) {
-            match parse_attr_action(String::from(line)) {
+            ActionKind::Attr => {
-                Ok(attr) => m.attributes.push(attr),
+                let attr = parse_attr_action(String::from(line))?;
-                Err(e) => return Err(e),
+                m.attributes.push(attr)
-            };
+            }
+            ActionKind::Dir => {
+
+            }
+            ActionKind::File => {
+
+            }
+            ActionKind::Dependency => {
+
+            }
+            ActionKind::User => {
+
+            }
+            ActionKind::Group => {
+
+            }
+            ActionKind::Driver => {
+
+            }
+            ActionKind::License => {
+
+            }
+            ActionKind::Link => {
+
+            }
+            ActionKind::Legacy => {
+
+            }
+            ActionKind::Unknown{action} => {
+                Err(ManifestError::UnknownAction {action, line: line_nr})?;
+            }
         }
     }
     return Ok(m);
 }
 
-fn is_attr_action(line: &String) -> bool {
+fn determine_action_kind(line: &str) -> ActionKind {
-    if line.trim().starts_with("set ") {
+    let mut act = String::new();
-        return true;
+    for c in line.trim_start().chars() {
+        if c == ' ' {
+            break
+        }
+        act.push(c)
+    }
+
+    return match act.as_str() {
+        "set" => ActionKind::Attr,
+        "depend" => ActionKind::Dependency,
+        "dir" => ActionKind::Dir,
+        "file" => ActionKind::File,
+        "license" => ActionKind::License,
+        "hardlink" => ActionKind::Link,
+        "link" => ActionKind::Link,
+        "driver" => ActionKind::Driver,
+        "group" => ActionKind::Group,
+        "user" => ActionKind::User,
+        "legacy" => ActionKind::Legacy,
+        _ => ActionKind::Unknown{action: act},
     }
-    return false;
 }
 
-pub fn parse_attr_action(line: String) -> Result<Attr, ManifestError> {
+pub fn parse_attr_action(line: String) -> Result<Attr, Error> {
     // Do a full line match to see if we can fast path this.
     // This also catches values with spaces, that have not been properly escaped.
     // Note: values with spaces must be properly escaped or the rest here will fail. Strings with
     //  unescaped spaces are never valid but sadly present in the wild.
     // Fast path will fail if a value has multiple values or a '=' sign in the values
-    let full_line_regex = match Regex::new(r"^set name=([^ ]+) value=(.+)$") {
+    let full_line_regex = Regex::new(r"^set name=([^ ]+) value=(.+)$")?;
-        Ok(re) => re,
-        Err(e) => return Err(ManifestError::Regex(e)),
-    };
 
     if full_line_regex.is_match(line.trim_start()) {
         match full_line_regex.captures(line.trim_start()) {
@@ -162,6 +214,7 @@ pub fn parse_attr_action(line: String) -> Result<Attr, ManifestError> {
                 }
 
                 val = val.replace(&['"', '\\'][..], "");
+                //TODO knock out single quotes somehow
 
                 if !fast_path_fail{
                     return Ok(Attr{
@@ -177,36 +230,21 @@ pub fn parse_attr_action(line: String) -> Result<Attr, ManifestError> {
 
 
     //Todo move regex initialisation out of for loop into static area
-    let name_regex = match Regex::new(r"name=([^ ]+) value=") {
+    let name_regex = Regex::new(r"name=([^ ]+) value=")?;
-        Ok(re) => re,
-        Err(e) => return Err(ManifestError::Regex(e)),
-    };
     let mut key = String::new();
     for cap in name_regex.captures_iter(line.trim_start()) {
         key = String::from(&cap[1]);
     }
 
     let mut values = Vec::new();
-    let value_no_space_regex = match Regex::new(r#"value="(.+)""#) {
+    let value_no_space_regex = Regex::new(r#"value="(.+)""#)?;
-        Ok(re) => re,
-        Err(e) => return Err(ManifestError::Regex(e)),
-    };
 
-    let value_space_regex = match Regex::new(r#"value=([^"][^ ]+[^"])"#) {
+    let value_space_regex = Regex::new(r#"value=([^"][^ ]+[^"])"#)?;
-        Ok(re) => re,
-        Err(e) => return Err(ManifestError::Regex(e)),
-    };
 
     let mut properties = HashSet::new();
-    let optionals_regex_no_quotes = match Regex::new(r#"([^ ]+)=([^"][^ ]+[^"])"#) {
+    let optionals_regex_no_quotes = Regex::new(r#"([^ ]+)=([^"][^ ]+[^"])"#)?;
-        Ok(re) => re,
-        Err(e) => return Err(ManifestError::Regex(e)),
-    };
 
-    let optionals_regex_quotes = match Regex::new(r#"([^ ]+)=([^"][^ ]+[^"])"#) {
+    let optionals_regex_quotes = Regex::new(r#"([^ ]+)=([^"][^ ]+[^"])"#)?;
-        Ok(re) => re,
-        Err(e) => return Err(ManifestError::Regex(e)),
-    };
 
     for cap in value_no_space_regex.captures_iter(line.trim_start()) {
         values.push(String::from(cap[1].trim()));

src/lib.rs

@@ -5,6 +5,8 @@
 
 mod actions;
 
+#[macro_use] extern crate failure;
+
 #[cfg(test)]
 mod tests {
 
@@ -28,7 +30,9 @@ mod tests {
         set name=pkg.summary value=\\\"provided mouse accessibility enhancements\\\"
         set name=info.upstream value=X.Org Foundation
         set name=pkg.description value=Latvian language support's extra files
-        set name=variant.arch value=i386 optional=testing optionalWithString=\"test ing\"");
+        set name=variant.arch value=i386 optional=testing optionalWithString=\"test ing\"
+        set name=info.source-url value=http://www.pgpool.net/download.php?f=pgpool-II-3.3.1.tar.gz
+        set name=pkg.summary value=\\\"'XZ Utils - loss-less file compression application and library.'\\\"");
 
         let mut optional_hash = HashSet::new();
         optional_hash.insert(Property{key: String::from("optional"), value:String::from("testing")});
@@ -109,16 +113,29 @@ mod tests {
                 key: String::from("variant.arch"),
                 values: vec![String::from("i386")],
                 properties: optional_hash,
+            },
+            Attr{
+                key: String::from("info.source-url"),
+                values: vec![String::from("http://www.pgpool.net/download.php?f=pgpool-II-3.3.1.tar.gz")],
+                properties: HashSet::new(),
+            },
+            Attr{
+                key: String::from("pkg.summary"),
+                values: vec![String::from("'XZ Utils - loss-less file compression application and library.'")], //TODO knock out the single quotes
+                properties: HashSet::new(),
             }
         ];
 
         let mut manifest = Manifest::new();
         match parse_manifest_string(manifest_string) {
             Ok(m) => manifest = m,
-            Err(_) => assert!(false, "caught error"),
+            Err(e) => {
+                println!("{}", e);
+                assert!(false, "caught error");
+            }
         };
 
-        assert_eq!(manifest.attributes.len(), 15);
+        assert_eq!(manifest.attributes.len(), 17);
 
         for (pos, attr) in manifest.attributes.iter().enumerate() {
             assert_eq!(attr.key, test_results[pos].key);