Improve SUMMARY parser error messages (fixes #566) (#567)

src/book/book.rs

@@ -61,11 +61,11 @@ fn create_missing(src_dir: &Path, summary: &Summary) -> Result<()> {
 
 /// A dumb tree structure representing a book.
 ///
-/// For the moment a book is just a collection of `BookItems` which are 
+/// For the moment a book is just a collection of `BookItems` which are
 /// accessible by either iterating (immutably) over the book with [`iter()`], or
 /// recursively applying a closure to each section to mutate the chapters, using
 /// [`for_each_mut()`].
-/// 
+///
 /// [`iter()`]: #method.iter
 /// [`for_each_mut()`]: #method.for_each_mut
 #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
@@ -199,7 +199,8 @@ fn load_chapter<P: AsRef<Path>>(link: &Link, src_dir: P) -> Result<Chapter> {
         .chain_err(|| format!("Chapter file not found, {}", link.location.display()))?;
 
     let mut content = String::new();
-    f.read_to_string(&mut content)?;
+    f.read_to_string(&mut content)
+        .chain_err(|| format!("Unable to read \"{}\" ({})", link.name, location.display()))?;
 
     let stripped = location
         .strip_prefix(&src_dir)
@@ -476,4 +477,43 @@ And here is some \
 
         assert_eq!(visited, num_items);
     }
+
+    #[test]
+    fn cant_load_chapters_with_an_empty_path() {
+        let (_, temp) = dummy_link();
+        let summary = Summary {
+            numbered_chapters: vec![
+                SummaryItem::Link(Link {
+                    name: String::from("Empty"),
+                    location: PathBuf::from(""),
+                    ..Default::default()
+                }),
+            ],
+            ..Default::default()
+        };
+
+        let got = load_book_from_disk(&summary, temp.path());
+        assert!(got.is_err());
+    }
+
+    #[test]
+    fn cant_load_chapters_when_the_link_is_a_directory() {
+        let (_, temp) = dummy_link();
+        let dir = temp.path().join("nested");
+        fs::create_dir(&dir).unwrap();
+
+        let summary = Summary {
+            numbered_chapters: vec![
+                SummaryItem::Link(Link {
+                    name: String::from("nested"),
+                    location: dir,
+                    ..Default::default()
+                }),
+            ],
+            ..Default::default()
+        };
+
+        let got = load_book_from_disk(&summary, temp.path());
+        assert!(got.is_err());
+    }
 }

src/book/summary.rs

@@ -6,7 +6,6 @@ use memchr::{self, Memchr};
 use pulldown_cmark::{self, Alignment, Event, Tag};
 use errors::*;
 
-
 /// Parse the text from a `SUMMARY.md` file into a sort of "recipe" to be
 /// used when loading a book from disk.
 ///
@@ -204,7 +203,7 @@ impl<'a> SummaryParser<'a> {
         }
     }
 
-    /// Get the current line and column to give the user more useful error 
+    /// Get the current line and column to give the user more useful error
     /// messages.
     fn current_location(&self) -> (usize, usize) {
         let byte_offset = self.stream.get_offset();
@@ -273,12 +272,16 @@ impl<'a> SummaryParser<'a> {
         let link_content = collect_events!(self.stream, end Tag::Link(..));
         let name = stringify_events(link_content);
 
-        Ok(Link {
+        if href.is_empty() {
-            name: name,
+            Err(self.parse_error("You can't have an empty link."))
-            location: PathBuf::from(href.to_string()),
+        } else {
-            number: None,
+            Ok(Link {
-            nested_items: Vec::new(),
+                name: name,
-        })
+                location: PathBuf::from(href.to_string()),
+                number: None,
+                nested_items: Vec::new(),
+            })
+        }
     }
 
     /// Parse the numbered chapters. This assumes the opening list tag has
@@ -325,7 +328,7 @@ impl<'a> SummaryParser<'a> {
                         //     break;
                         // }
                         if let Event::End(tag) = event {
-                            if tag_eq(&tag, &other_tag) { 
+                            if tag_eq(&tag, &other_tag) {
                                 break;
                             }
                         }
@@ -484,23 +487,25 @@ fn stringify_events(events: Vec<Event>) -> String {
 // FIXME: Remove this when google/pulldown_cmark#120 lands (new patch release)
 fn tag_eq(left: &Tag, right: &Tag) -> bool {
     match (left, right) {
-    (&Tag::Paragraph, &Tag::Paragraph) => true,
+        (&Tag::Paragraph, &Tag::Paragraph) => true,
-    (&Tag::Rule, &Tag::Rule) => true,
+        (&Tag::Rule, &Tag::Rule) => true,
-    (&Tag::Header(a), &Tag::Header(b)) => a == b,
+        (&Tag::Header(a), &Tag::Header(b)) => a == b,
-    (&Tag::BlockQuote, &Tag::BlockQuote) => true,
+        (&Tag::BlockQuote, &Tag::BlockQuote) => true,
-    (&Tag::CodeBlock(ref a), &Tag::CodeBlock(ref b)) => a == b,
+        (&Tag::CodeBlock(ref a), &Tag::CodeBlock(ref b)) => a == b,
-    (&Tag::List(ref a), &Tag::List(ref b)) => a == b,
+        (&Tag::List(ref a), &Tag::List(ref b)) => a == b,
-    (&Tag::Item, &Tag::Item) => true,
+        (&Tag::Item, &Tag::Item) => true,
-    (&Tag::FootnoteDefinition(ref a), &Tag::FootnoteDefinition(ref b)) => a == b,
+        (&Tag::FootnoteDefinition(ref a), &Tag::FootnoteDefinition(ref b)) => a == b,
-    (&Tag::Table(ref a), &Tag::Table(ref b)) => a.iter().zip(b.iter()).all(|(l, r)| alignment_eq(*l, *r)),
+        (&Tag::Table(ref a), &Tag::Table(ref b)) => {
-    (&Tag::TableHead, &Tag::TableHead) => true,
+            a.iter().zip(b.iter()).all(|(l, r)| alignment_eq(*l, *r))
-    (&Tag::TableRow, &Tag::TableRow) => true,
+        }
-    (&Tag::TableCell, &Tag::TableCell) => true,
+        (&Tag::TableHead, &Tag::TableHead) => true,
-    (&Tag::Emphasis, &Tag::Emphasis) => true,
+        (&Tag::TableRow, &Tag::TableRow) => true,
-    (&Tag::Strong, &Tag::Strong) => true,
+        (&Tag::TableCell, &Tag::TableCell) => true,
-    (&Tag::Code, &Tag::Code) => true,
+        (&Tag::Emphasis, &Tag::Emphasis) => true,
-    (&Tag::Link(ref a_1, ref a_2), &Tag::Link(ref b_1, ref b_2)) => a_1 == b_1 && a_2 == b_2,
+        (&Tag::Strong, &Tag::Strong) => true,
-    (&Tag::Image(ref a_1, ref a_2), &Tag::Image(ref b_1, ref b_2)) => a_1 == b_1 && a_2 == b_2,
+        (&Tag::Code, &Tag::Code) => true,
+        (&Tag::Link(ref a_1, ref a_2), &Tag::Link(ref b_1, ref b_2)) => a_1 == b_1 && a_2 == b_2,
+        (&Tag::Image(ref a_1, ref a_2), &Tag::Image(ref b_1, ref b_2)) => a_1 == b_1 && a_2 == b_2,
         _ => false,
     }
 }
@@ -512,7 +517,7 @@ fn alignment_eq(left: Alignment, right: Alignment) -> bool {
         (Alignment::Left, Alignment::Left) => true,
         (Alignment::Center, Alignment::Center) => true,
         (Alignment::Right, Alignment::Right) => true,
-        _ => false
+        _ => false,
     }
 }
 
@@ -754,4 +759,14 @@ mod tests {
 
         assert_eq!(got, should_be);
     }
-}
+
+    #[test]
+    fn an_empty_link_location_is_an_error() {
+        let src = "- [Empty]()\n";
+        let mut parser = SummaryParser::new(src);
+        parser.stream.next();
+
+        let got = parser.parse_numbered();
+        assert!(got.is_err());
+    }
+}