Better error handling (fixes #34)

- Uses liquid 0.2, which does better error handling
- Introduces a cobalt::Error that converts from the different error
  types and enables much conciser and safer code
- Transitions all panics and unwraps to use the new error system

Cargo.toml

@@ -7,7 +7,7 @@ authors = ["Benny Klotz <r3qnbenni@gmail.com>", "Johann Hofmann"]
 [dependencies]
 getopts = "0.2.14"
 markdown = "0.1"
-liquid = "0.1"
+liquid = "0.2"
 walkdir = "0.1"
 crossbeam = "0.1.5"
 

src/cobalt.rs

@@ -2,38 +2,24 @@ use crossbeam;
 
 use std::sync::Arc;
 use std::fs::{self, File};
-use std::io::{self, Read};
+use std::io::{Read};
 use std::path::Path;
 use std::collections::HashMap;
 use std::ffi::OsStr;
 use liquid::Value;
 use walkdir::WalkDir;
 use document::Document;
+use error::Result;
 
-pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> io::Result<()> {
+/// The primary build function that tranforms a directory into a site
+pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> Result<()> {
     // TODO make configurable
     let template_extensions = [OsStr::new("tpl"), OsStr::new("md")];
 
     let layouts_path = source.join(layout_str);
     let posts_path = source.join(posts_str);
 
-    let mut layouts: HashMap<String, String> = HashMap::new();
-
-    let walker = WalkDir::new(&layouts_path).into_iter();
-
-    // go through the layout directory and add
-    // filename -> text content to the layout map
-    for entry in walker.filter_map(|e| e.ok()).filter(|e| e.file_type().is_file()) {
-        let mut text = String::new();
-        try!(File::open(entry.path()).expect(&format!("Failed to open file {:?}", entry)).read_to_string(&mut text));
-        layouts.insert(entry.path()
-                            .file_name()
-                            .expect(&format!("No file name from {:?}", entry))
-                            .to_str()
-                            .expect(&format!("Invalid UTF-8 in {:?}", entry))
-                            .to_owned(),
-                       text);
-    }
+    let layouts = try!(get_layouts(&layouts_path));
 
     let mut documents = vec![];
     let mut post_data = vec![];
@@ -45,7 +31,7 @@ pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> i
                                               .extension()
                                               .unwrap_or(OsStr::new(""))) &&
            entry.path().parent() != Some(layouts_path.as_path()) {
-            let doc = parse_document(&entry.path(), source);
+            let doc = try!(parse_document(&entry.path(), source));
             if entry.path().parent() == Some(posts_path.as_path()) {
                 post_data.push(Value::Object(doc.get_attributes()));
             }
@@ -63,9 +49,7 @@ pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> i
         for doc in &documents {
             let post_data = post_data.clone();
             let layouts = layouts.clone();
-            let handle = scope.spawn(move || {
-                doc.create_file(dest, &layouts, &post_data)
-            });
+            let handle = scope.spawn(move || doc.create_file(dest, &layouts, &post_data));
             handles.push(handle);
         }
     });
@@ -79,7 +63,6 @@ pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> i
         let walker = WalkDir::new(&source)
                          .into_iter()
                          .filter_map(|e| e.ok())
-                         // filter out files to not copy
                          .filter(|f| {
                              let p = f.path();
                              // don't copy hidden files
@@ -88,19 +71,19 @@ pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> i
                                .to_str()
                                .unwrap_or("")
                                .starts_with(".") &&
-                             // don't copy templates
-                             !template_extensions.contains(&p.extension().unwrap_or(OsStr::new(""))) &&
-                             // this is madness
-                             p != dest &&
-                             // don't copy from the layouts folder
-                             p != layouts_path.as_path()
+                             !template_extensions.contains(&p.extension()
+                                                             .unwrap_or(OsStr::new(""))) &&
+                             p != dest && p != layouts_path.as_path()
                          });
 
         for entry in walker {
             let relative = entry.path()
-                                .to_str().expect(&format!("Invalid UTF-8 in {:?}", entry))
-                                .split(source.to_str().expect(&format!("Invalid UTF-8 in {:?}", source)))
-                                .last().expect(&format!("Empty path"));
+                                .to_str()
+                                .expect(&format!("Invalid UTF-8 in {:?}", entry))
+                                .split(source.to_str()
+                                             .expect(&format!("Invalid UTF-8 in {:?}", source)))
+                                .last()
+                                .expect(&format!("Empty path"));
 
             if try!(entry.metadata()).is_dir() {
                 try!(fs::create_dir_all(&dest.join(relative)));
@@ -113,9 +96,39 @@ pub fn build(source: &Path, dest: &Path, layout_str: &str, posts_str: &str) -> i
     Ok(())
 }
 
-fn parse_document(path: &Path, source: &Path) -> Document {
-    let attributes = extract_attributes(path);
-    let content = extract_content(path).expect(&format!("No content in {:?}", path));
+/// Gets all layout files from the specified path (usually _layouts/)
+/// This walks the specified directory recursively
+///
+/// Returns a map filename -> content
+fn get_layouts(layouts_path: &Path) -> Result<HashMap<String, String>> {
+    let mut layouts = HashMap::new();
+
+    let walker = WalkDir::new(layouts_path).into_iter();
+
+    // go through the layout directory and add
+    // filename -> text content to the layout map
+    for entry in walker.filter_map(|e| e.ok()).filter(|e| e.file_type().is_file()) {
+        let mut text = String::new();
+        let mut file = try!(File::open(entry.path()));
+        try!(file.read_to_string(&mut text));
+
+        let path = try!(entry.path()
+                             .file_name()
+                             .and_then(|name| name.to_str())
+                             .ok_or(format!("Cannot convert pathname {:?} to UTF-8",
+                                            entry.path().file_name())));
+
+        layouts.insert(path.to_owned(), text);
+    }
+
+    Ok(layouts)
+}
+
+
+fn parse_document(path: &Path, source: &Path) -> Result<Document> {
+    let attributes = try!(extract_attributes(path));
+    let content = try!(extract_content(path));
+
     let new_path = path.to_str()
                        .expect(&format!("Invalid UTF-8 in {:?}", path))
                        .split(source.to_str()
@@ -124,17 +137,17 @@ fn parse_document(path: &Path, source: &Path) -> Document {
                        .expect(&format!("Empty path"));
     let markdown = path.extension().unwrap_or(OsStr::new("")) == OsStr::new("md");
 
-    Document::new(new_path.to_owned(), attributes, content, markdown)
+    Ok(Document::new(new_path.to_owned(), attributes, content, markdown))
 }
 
-fn parse_file(path: &Path) -> io::Result<String> {
+fn parse_file(path: &Path) -> Result<String> {
     let mut file = try!(File::open(path));
     let mut text = String::new();
     try!(file.read_to_string(&mut text));
     Ok(text)
 }
 
-fn extract_attributes(path: &Path) -> HashMap<String, String> {
+fn extract_attributes(path: &Path) -> Result<HashMap<String, String>> {
     let mut attributes = HashMap::new();
     attributes.insert("name".to_owned(),
                       path.file_stem()
@@ -148,7 +161,7 @@ fn extract_attributes(path: &Path) -> HashMap<String, String> {
     if content.contains("---") {
         let mut content_splits = content.split("---");
 
-        let attribute_string = content_splits.nth(0).expect(&format!("Empty content"));
+        let attribute_string = try!(content_splits.nth(0).ok_or("Empty content"));
 
         for attribute_line in attribute_string.split("\n") {
             if !attribute_line.contains(':') {
@@ -164,16 +177,16 @@ fn extract_attributes(path: &Path) -> HashMap<String, String> {
         }
     }
 
-    return attributes;
+    Ok(attributes)
 }
 
-fn extract_content(path: &Path) -> io::Result<String> {
+fn extract_content(path: &Path) -> Result<String> {
     let content = try!(parse_file(path));
 
     if content.contains("---") {
         let mut content_splits = content.split("---");
 
-        return Ok(content_splits.nth(1).expect(&format!("No content after header")).to_owned());
+        return Ok(try!(content_splits.nth(1).ok_or("No content after header")).to_owned());
     }
 
     return Ok(content);

src/document.rs

@@ -1,9 +1,9 @@
-use std::{io, fs};
-use std::fs::File;
+use std::fs::{self, File};
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 use std::default::Default;
 use std::io::Write;
+use error::Result;
 
 use liquid::{Renderable, LiquidOptions, Context, Value};
 
@@ -42,22 +42,23 @@ impl Document {
         data
     }
 
-    pub fn as_html(&self, post_data: &Vec<Value>) -> Result<String, String> {
+    pub fn as_html(&self, post_data: &Vec<Value>) -> Result<String> {
         let mut options: LiquidOptions = Default::default();
         let template = try!(liquid::parse(&self.content, &mut options));
 
-        // TODO: pass in documents as template data if as_html is called on Index Document..
+        // TODO: pass in documents as template data if as_html is called on Index
+        // Document..
         let mut data = Context::with_values(self.get_attributes());
         data.set_val("posts", Value::Array(post_data.clone()));
 
-        Ok(template.render(&mut data).unwrap_or(String::new()))
+        Ok(try!(template.render(&mut data)).unwrap_or(String::new()))
     }
 
     pub fn create_file(&self,
                        dest: &Path,
                        layouts: &HashMap<String, String>,
                        post_data: &Vec<Value>)
-                       -> io::Result<()> {
+                       -> Result<()> {
         // construct target path
         let mut file_path_buf = PathBuf::new();
         file_path_buf.push(dest);
@@ -66,30 +67,29 @@ impl Document {
 
         let file_path = file_path_buf.as_path();
 
-        let layout_path = self.attributes.get(&"@extends".to_owned()).expect(&format!("No @extends line creating {:?}", self.name));
-        let layout = layouts.get(layout_path).expect(&format!("No layout path {:?} creating {:?}", layout_path, self.name));
+        let layout_path = try!(self.attributes
+                                   .get(&"@extends".to_owned())
+                                   .ok_or(format!("No @extends line creating {}", self.name)));
+
+        let layout = try!(layouts.get(layout_path)
+                                 .ok_or(format!("No layout path {} creating {}",
+                                                layout_path,
+                                                self.name)));
 
         // create target directories if any exist
-        match file_path.parent() {
-            Some(ref parents) => try!(fs::create_dir_all(parents)),
-            None => (),
-        };
+        file_path.parent().map(|p| fs::create_dir_all(p));
 
         let mut file = try!(File::create(&file_path));
 
         let mut data = Context::new();
 
-        // TODO: improve error handling for liquid errors
-        let mut html = match self.as_html(post_data) {
-            Ok(x) => x,
-            Err(e) => {
-                println!("Warning, liquid failed: {}", e);
-                String::new()
-            }
-        };
+        // compile with liquid
+        let mut html = try!(self.as_html(post_data));
+
         if self.markdown {
             html = markdown::to_html(&html);
         }
+
         data.set_val("content", Value::Str(html));
 
         // Insert the attributes into the layout template
@@ -100,17 +100,13 @@ impl Document {
         }
 
         let mut options: LiquidOptions = Default::default();
-        // TODO: improve error handling for liquid errors
-        let template = match liquid::parse(&layout, &mut options) {
-            Ok(x) => x,
-            Err(e) => {
-                panic!("Warning, liquid failed: {}", e);
-            }
-        };
 
-        let res = template.render(&mut data).unwrap_or(String::new());
+        let template = try!(liquid::parse(&layout, &mut options));
 
+        let res = try!(template.render(&mut data)).unwrap_or(String::new());
+
+        try!(file.write_all(&res.into_bytes()));
         println!("Created {}", file_path.display());
-        file.write_all(&res.into_bytes())
+        Ok(())
     }
 }

src/error.rs

@@ -0,0 +1,78 @@
+use std::result;
+use std::io;
+use std::error;
+use std::fmt;
+use walkdir;
+use liquid;
+
+// type alias because we always want to deal with CobaltErrors
+pub type Result<T> = result::Result<T, Error>;
+
+#[derive(Debug)]
+pub enum Error {
+    Io(io::Error),
+    Liquid(liquid::Error),
+    WalkDir(walkdir::Error),
+    Other(String),
+}
+
+impl From<io::Error> for Error {
+    fn from(err: io::Error) -> Error {
+        Error::Io(err)
+    }
+}
+
+impl From<liquid::Error> for Error {
+    fn from(err: liquid::Error) -> Error {
+        Error::Liquid(err)
+    }
+}
+
+impl From<walkdir::Error> for Error {
+    fn from(err: walkdir::Error) -> Error {
+        Error::WalkDir(err)
+    }
+}
+
+impl From<String> for Error {
+    fn from(err: String) -> Error {
+        Error::Other(err)
+    }
+}
+
+impl<'a> From<&'a str> for Error {
+    fn from(err: &'a str) -> Error {
+        Error::Other(err.to_owned())
+    }
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            Error::Io(ref err) => write!(f, "IO error: {}", err),
+            Error::Liquid(ref err) => write!(f, "Liquid error: {}", err),
+            Error::WalkDir(ref err) => write!(f, "walkdir error: {}", err),
+            Error::Other(ref err) => write!(f, "error: {}", err),
+        }
+    }
+}
+
+impl error::Error for Error {
+    fn description(&self) -> &str {
+        match *self {
+            Error::Io(ref err) => err.description(),
+            Error::Liquid(ref err) => err.description(),
+            Error::WalkDir(ref err) => err.description(),
+            Error::Other(ref err) => err,
+        }
+    }
+
+    fn cause(&self) -> Option<&error::Error> {
+        match *self {
+            Error::Io(ref err) => Some(err),
+            Error::Liquid(ref err) => Some(err),
+            Error::WalkDir(ref err) => Some(err),
+            Error::Other(_) => None,
+        }
+    }
+}

src/lib.rs

@@ -5,10 +5,10 @@ extern crate markdown;
 extern crate walkdir;
 extern crate crossbeam;
 
-// without this main.rs would have to use cobalt::cobalt
-// with this approach you can explicitly say which part of a module is public and which not
 pub use cobalt::build;
+pub use error::Error;
 
 // modules
 mod cobalt;
+mod error;
 mod document;

tests/fixtures/liquid_error/_layouts/default.tpl

@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <title>test</title>
+    </head>
+    <body>
+        <h1>{{ name }}</h1>
+
+        {{ content }}
+    </body>
+</html>
+

tests/fixtures/liquid_error/_layouts/posts.tpl

@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <title>My blog - {{ title }}</title>
+    </head>
+    <body>
+        {{ content }}
+    </body>
+</html>
+

tests/fixtures/liquid_error/_posts/2014-08-24-my-first-blogpost.md

@@ -0,0 +1,10 @@
+@extends: posts.tpl
+
+title:   My first Blogpost
+date:    24/08/2014 at 15:36
+---
+# {{ title }}
+
+Hey there this is my first blogpost and this is super awesome.
+
+My Blog is lorem ipsum like, yes it is..

tests/fixtures/liquid_error/index.tpl

@@ -0,0 +1,7 @@
+@extends: default.tpl
+---
+This is my Index page!
+
+{% for post in posts %}
+ {{ post.title }}
+{{{{{}}}}% endfor %}

tests/fixtures/no_extends_error/_layouts/default.tpl

@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <title>test</title>
+    </head>
+    <body>
+        <h1>{{ name }}</h1>
+
+        {{ content }}
+    </body>
+</html>
+

tests/fixtures/no_extends_error/_layouts/posts.tpl

@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <title>My blog - {{ title }}</title>
+    </head>
+    <body>
+        {{ content }}
+    </body>
+</html>
+

tests/fixtures/no_extends_error/_posts/2014-08-24-my-first-blogpost.md

@@ -0,0 +1,8 @@
+title:   My first Blogpost
+date:    24/08/2014 at 15:36
+---
+# {{ title }}
+
+Hey there this is my first blogpost and this is super awesome.
+
+My Blog is lorem ipsum like, yes it is..

tests/fixtures/no_extends_error/index.tpl

@@ -0,0 +1,7 @@
+@extends: default.tpl
+---
+This is my Index page!
+
+{% for post in posts %}
+ {{ post.title }}
+{% endfor %}

tests/mod.rs

@@ -6,39 +6,56 @@ use std::path::Path;
 use std::fs::{self, File};
 use std::io::Read;
 use walkdir::WalkDir;
+use std::error::Error;
 
-fn run_test(name: &str) {
+fn run_test(name: &str) -> Result<(), cobalt::Error> {
     let source = format!("tests/fixtures/{}/", name);
     let target = format!("tests/target/{}/", name);
     let dest = format!("tests/tmp/{}/", name);
 
-    match cobalt::build(&Path::new(&source), &Path::new(&dest), "_layouts", "_posts") {
-        Ok(_) => println!("Build successful"),
-        Err(e) => panic!("Error: {}", e),
+    let result = cobalt::build(&Path::new(&source), &Path::new(&dest), "_layouts", "_posts");
+
+    if result.is_ok() {
+        let walker = WalkDir::new(&target).into_iter();
+
+        // walk through fixture and created tmp directory and compare files
+        for entry in walker.filter_map(|e| e.ok()).filter(|e| e.file_type().is_file()) {
+            let relative = entry.path().to_str().unwrap().split(&target).last().unwrap();
+
+            let mut original = String::new();
+            File::open(entry.path()).unwrap().read_to_string(&mut original).unwrap();
+
+            let mut created = String::new();
+            File::open(&Path::new(&dest).join(&relative))
+                .unwrap()
+                .read_to_string(&mut created)
+                .unwrap();
+
+            difference::assert_diff(&original, &created, " ", 0);
+        }
+
+        // clean up
+        fs::remove_dir_all(dest).expect("Cleanup failed");
     }
 
-    let walker = WalkDir::new(&target).into_iter();
-
-    // walk through fixture and created tmp directory and compare files
-    for entry in walker.filter_map(|e| e.ok()).filter(|e| e.file_type().is_file()) {
-        let relative = entry.path().to_str().unwrap().split(&target).last().unwrap();
-
-        let mut original = String::new();
-        File::open(entry.path()).unwrap().read_to_string(&mut original).unwrap();
-
-        println!("{:?}", &Path::new(&dest).join(&relative));
-        let mut created = String::new();
-        File::open(&Path::new(&dest).join(&relative)).unwrap().read_to_string(&mut created).unwrap();
-
-        difference::assert_diff(&original, &created, " ", 0);
-    }
-
-    // clean up
-    fs::remove_dir_all(dest).unwrap();
+    result
 }
 
 #[test]
 pub fn example() {
-    run_test("example");
+    assert!(run_test("example").is_ok());
 }
 
+#[test]
+pub fn liquid_error() {
+    let err = run_test("liquid_error");
+    assert!(err.is_err());
+    assert_eq!(err.unwrap_err().description(), "{{{ is not a valid identifier");
+}
+
+#[test]
+pub fn no_extends_error() {
+    let err = run_test("no_extends_error");
+    assert!(err.is_err());
+    assert_eq!(err.unwrap_err().description(), "No @extends line creating _posts/2014-08-24-my-first-blogpost.md");
+}