Recursively apply replace_all() when running the links preprocessor (#564)

* Looks like we forgot to recursively apply replace_all() in #532

* Removed some print statements

* Made sure we ignore the rendered dummy_book

.gitignore

@@ -8,3 +8,5 @@ book-test
 book-example/book
 
 .vscode
+tests/dummy_book/book/
+

src/book/book.rs

@@ -8,7 +8,6 @@ use super::summary::{parse_summary, Link, SectionNumber, Summary, SummaryItem};
 use config::BuildConfig;
 use errors::*;
 
-
 /// Load a book into memory from its `src/` directory.
 pub fn load_book<P: AsRef<Path>>(src_dir: P, cfg: &BuildConfig) -> Result<Book> {
     let src_dir = src_dir.as_ref();
@@ -60,14 +59,19 @@ fn create_missing(src_dir: &Path, summary: &Summary) -> Result<()> {
     Ok(())
 }
 
-
 /// A dumb tree structure representing a book.
 ///
-/// For the moment a book is just a collection of `BookItems`.
+/// 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)]
 pub struct Book {
     /// The sections in this book.
-    pub sections: Vec<BookItem>,
+    sections: Vec<BookItem>,
 }
 
 impl Book {
@@ -82,6 +86,35 @@ impl Book {
             items: self.sections.iter().collect(),
         }
     }
+
+    /// Recursively apply a closure to each item in the book, allowing you to
+    /// mutate them.
+    ///
+    /// # Note
+    ///
+    /// Unlike the `iter()` method, this requires a closure instead of returning
+    /// an iterator. This is because using iterators can possibly allow you
+    /// to have iterator invalidation errors.
+    pub fn for_each_mut<F>(&mut self, mut func: F)
+    where
+        F: FnMut(&mut BookItem),
+    {
+        for_each_mut(&mut func, &mut self.sections);
+    }
+}
+
+pub fn for_each_mut<'a, F, I>(func: &mut F, items: I)
+where
+    F: FnMut(&mut BookItem),
+    I: IntoIterator<Item = &'a mut BookItem>,
+{
+    for item in items {
+        if let &mut BookItem::Chapter(ref mut ch) = item {
+            for_each_mut(func, &mut ch.sub_items);
+        }
+
+        func(item);
+    }
 }
 
 /// Enum representing any type of item which can be added to a book.
@@ -224,7 +257,6 @@ impl Display for Chapter {
     }
 }
 
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -266,7 +298,6 @@ And here is some \
             .write_all("Hello World!".as_bytes())
             .unwrap();
 
-
         let mut second = Link::new("Nested Chapter 1", &second_path);
         second.number = Some(SectionNumber(vec![1, 2]));
 
@@ -391,7 +422,6 @@ And here is some \
             ],
         };
 
-
         let got: Vec<_> = book.iter().collect();
 
         assert_eq!(got.len(), 5);
@@ -411,4 +441,39 @@ And here is some \
 
         assert_eq!(chapter_names, should_be);
     }
+
+    #[test]
+    fn for_each_mut_visits_all_items() {
+        let mut book = Book {
+            sections: vec![
+                BookItem::Chapter(Chapter {
+                    name: String::from("Chapter 1"),
+                    content: String::from(DUMMY_SRC),
+                    number: None,
+                    path: PathBuf::from("Chapter_1/index.md"),
+                    sub_items: vec![
+                        BookItem::Chapter(Chapter::new(
+                            "Hello World",
+                            String::new(),
+                            "Chapter_1/hello.md",
+                        )),
+                        BookItem::Separator,
+                        BookItem::Chapter(Chapter::new(
+                            "Goodbye World",
+                            String::new(),
+                            "Chapter_1/goodbye.md",
+                        )),
+                    ],
+                }),
+                BookItem::Separator,
+            ],
+        };
+
+        let num_items = book.iter().count();
+        let mut visited = 0;
+
+        book.for_each_mut(|_| visited += 1);
+
+        assert_eq!(visited, num_items);
+    }
 }

src/preprocess/links.rs

@@ -1,4 +1,4 @@
-use std::ops::{Range, RangeFrom, RangeTo, RangeFull};
+use std::ops::{Range, RangeFrom, RangeFull, RangeTo};
 use std::path::{Path, PathBuf};
 use regex::{CaptureMatches, Captures, Regex};
 use utils::fs::file_to_string;
@@ -29,38 +29,49 @@ impl Preprocessor for LinkPreprocessor {
     fn run(&self, ctx: &PreprocessorContext, book: &mut Book) -> Result<()> {
         let src_dir = ctx.root.join(&ctx.config.book.src);
 
-        for section in &mut book.sections {
+        book.for_each_mut(|section: &mut BookItem| {
-            match *section {
+            if let BookItem::Chapter(ref mut ch) = *section {
-                BookItem::Chapter(ref mut ch) => {
+                let base = ch.path
-                    let base = ch.path.parent()
+                    .parent()
                     .map(|dir| src_dir.join(dir))
-                        .ok_or_else(|| String::from("Invalid bookitem path!"))?;
+                    .expect("All book items have a parent");
-                    let content = replace_all(&ch.content, base)?;
+
-                    ch.content = content
+                let content = replace_all(&ch.content, base);
-                }
+                ch.content = content;
-                _ => {}
-            }
             }
+        });
 
         Ok(())
     }
 }
 
-fn replace_all<P: AsRef<Path>>(s: &str, path: P) -> Result<String> {
+fn replace_all<P: AsRef<Path>>(s: &str, path: P) -> String {
     // When replacing one thing in a string by something with a different length,
     // the indices after that will not correspond,
     // we therefore have to store the difference to correct this
+    let path = path.as_ref();
     let mut previous_end_index = 0;
     let mut replaced = String::new();
 
     for playpen in find_links(s) {
         replaced.push_str(&s[previous_end_index..playpen.start_index]);
-        replaced.push_str(&playpen.render_with_path(&path)?);
+
+        match playpen.render_with_path(&path) {
+            Ok(new_content) => {
+                replaced.push_str(&new_content);
                 previous_end_index = playpen.end_index;
             }
+            Err(e) => {
+                error!("Error updating \"{}\", {}", playpen.link_text, e);
+                // This should make sure we include the raw `{{# ... }}` snippet
+                // in the page content if there are any errors.
+                previous_end_index = playpen.start_index;
+            }
+        }
+    }
 
     replaced.push_str(&s[previous_end_index..]);
-    Ok(replaced)
+    replaced
 }
 
 #[derive(PartialEq, Debug, Clone)]
@@ -79,18 +90,20 @@ fn parse_include_path(path: &str) -> LinkType<'static> {
     let start = parts.next().and_then(|s| s.parse::<usize>().ok());
     let end = parts.next().and_then(|s| s.parse::<usize>().ok());
     match start {
-        Some(start) => {
+        Some(start) => match end {
-            match end {
+            Some(end) => LinkType::IncludeRange(
-                Some(end) => LinkType::IncludeRange(path, Range{ start: start, end: end}),
+                path,
+                Range {
+                    start: start,
+                    end: end,
+                },
+            ),
             None => LinkType::IncludeRangeFrom(path, RangeFrom { start: start }),
-            }
+        },
-        }
+        None => match end {
-        None => {
-            match end {
             Some(end) => LinkType::IncludeRangeTo(path, RangeTo { end: end }),
             None => LinkType::IncludeRangeFull(path, RangeFull),
-            }
+        },
-        }
     }
 }
 
@@ -116,20 +129,18 @@ impl<'a> Link<'a> {
                     _ => None,
                 }
             }
-            (Some(mat), None, None) if mat.as_str().starts_with(ESCAPE_CHAR) => Some(
+            (Some(mat), None, None) if mat.as_str().starts_with(ESCAPE_CHAR) => {
-                LinkType::Escaped,
+                Some(LinkType::Escaped)
-            ),
+            }
             _ => None,
         };
 
         link_type.and_then(|lnk| {
-            cap.get(0).map(|mat| {
+            cap.get(0).map(|mat| Link {
-                Link {
                 start_index: mat.start(),
                 end_index: mat.end(),
                 link: lnk,
                 link_text: mat.as_str(),
-                }
             })
         })
     }
@@ -139,37 +150,20 @@ impl<'a> Link<'a> {
         match self.link {
             // omit the escape char
             LinkType::Escaped => Ok((&self.link_text[1..]).to_owned()),
-            LinkType::IncludeRange(ref pat, ref range) => {
+            LinkType::IncludeRange(ref pat, ref range) => file_to_string(base.join(pat))
-                file_to_string(base.join(pat))
                 .map(|s| take_lines(&s, range.clone()))
-                    .chain_err(|| {
+                .chain_err(|| format!("Could not read file for link {}", self.link_text)),
-                        format!("Could not read file for link {}", self.link_text)
+            LinkType::IncludeRangeFrom(ref pat, ref range) => file_to_string(base.join(pat))
-                    })
-            }
-            LinkType::IncludeRangeFrom(ref pat, ref range) => {
-                file_to_string(base.join(pat))
                 .map(|s| take_lines(&s, range.clone()))
-                    .chain_err(|| {
+                .chain_err(|| format!("Could not read file for link {}", self.link_text)),
-                        format!("Could not read file for link {}", self.link_text)
+            LinkType::IncludeRangeTo(ref pat, ref range) => file_to_string(base.join(pat))
-                    })
-            }
-            LinkType::IncludeRangeTo(ref pat, ref range) => {
-                file_to_string(base.join(pat))
                 .map(|s| take_lines(&s, range.clone()))
-                    .chain_err(|| {
+                .chain_err(|| format!("Could not read file for link {}", self.link_text)),
-                        format!("Could not read file for link {}", self.link_text)
+            LinkType::IncludeRangeFull(ref pat, _) => file_to_string(base.join(pat))
-                    })
+                .chain_err(|| format!("Could not read file for link {}", self.link_text)),
-            }
-            LinkType::IncludeRangeFull(ref pat, _) => {
-                file_to_string(base.join(pat))
-                    .chain_err(|| {
-                        format!("Could not read file for link {}", self.link_text)
-                    })
-            }
             LinkType::Playpen(ref pat, ref attrs) => {
-                let contents = file_to_string(base.join(pat)).chain_err(|| {
+                let contents = file_to_string(base.join(pat))
-                    format!("Could not read file for link {}", self.link_text)
+                    .chain_err(|| format!("Could not read file for link {}", self.link_text))?;
-                })?;
                 let ftype = if !attrs.is_empty() { "rust," } else { "rust" };
                 Ok(format!(
                     "```{}{}\n{}\n```\n",
@@ -213,9 +207,9 @@ fn find_links(contents: &str) -> LinkIter {
     LinkIter(RE.captures_iter(contents))
 }
 
-// ---------------------------------------------------------------------------------
+#[cfg(test)]
-//      Tests
+mod tests {
-//
+    use super::*;
 
     #[test]
     fn test_find_links_no_link() {
@@ -252,8 +246,10 @@ fn test_find_links_simple_link() {
         let res = find_links(s).collect::<Vec<_>>();
         println!("\nOUTPUT: {:?}\n", res);
 
-    assert_eq!(res,
+        assert_eq!(
-               vec![Link {
+            res,
+            vec![
+                Link {
                     start_index: 22,
                     end_index: 42,
                     link: LinkType::Playpen(PathBuf::from("file.rs"), vec![]),
@@ -264,7 +260,9 @@ fn test_find_links_simple_link() {
                     end_index: 68,
                     link: LinkType::Playpen(PathBuf::from("test.rs"), vec![]),
                     link_text: "{{#playpen test.rs }}",
-                    }]);
+                },
+            ]
+        );
     }
 
     #[test]
@@ -339,6 +337,24 @@ fn test_find_links_with_full_range() {
         );
     }
 
+    #[test]
+    fn test_find_links_with_no_range_specified() {
+        let s = "Some random text with {{#include file.rs}}...";
+        let res = find_links(s).collect::<Vec<_>>();
+        println!("\nOUTPUT: {:?}\n", res);
+        assert_eq!(
+            res,
+            vec![
+                Link {
+                    start_index: 22,
+                    end_index: 42,
+                    link: LinkType::IncludeRangeFull(PathBuf::from("file.rs"), ..),
+                    link_text: "{{#include file.rs}}",
+                },
+            ]
+        );
+    }
+
     #[test]
     fn test_find_links_escaped_link() {
         let s = "Some random text with escaped playpen \\{{#playpen file.rs editable}} ...";
@@ -346,24 +362,30 @@ fn test_find_links_escaped_link() {
         let res = find_links(s).collect::<Vec<_>>();
         println!("\nOUTPUT: {:?}\n", res);
 
-    assert_eq!(res,
+        assert_eq!(
-               vec![Link {
+            res,
+            vec![
+                Link {
                     start_index: 38,
                     end_index: 68,
                     link: LinkType::Escaped,
                     link_text: "\\{{#playpen file.rs editable}}",
-                    }]);
+                },
+            ]
+        );
     }
 
     #[test]
     fn test_find_playpens_with_properties() {
-    let s = "Some random text with escaped playpen {{#playpen file.rs editable }} and some more\n \
+        let s = "Some random text with escaped playpen {{#playpen file.rs editable }} and some \
-             text {{#playpen my.rs editable no_run should_panic}} ...";
+                 more\n text {{#playpen my.rs editable no_run should_panic}} ...";
 
         let res = find_links(s).collect::<Vec<_>>();
         println!("\nOUTPUT: {:?}\n", res);
-    assert_eq!(res,
+        assert_eq!(
-               vec![Link {
+            res,
+            vec![
+                Link {
                     start_index: 38,
                     end_index: 68,
                     link: LinkType::Playpen(PathBuf::from("file.rs"), vec!["editable"]),
@@ -372,41 +394,55 @@ fn test_find_playpens_with_properties() {
                 Link {
                     start_index: 89,
                     end_index: 136,
-                        link: LinkType::Playpen(PathBuf::from("my.rs"),
+                    link: LinkType::Playpen(
-                                                vec!["editable", "no_run", "should_panic"]),
+                        PathBuf::from("my.rs"),
+                        vec!["editable", "no_run", "should_panic"],
+                    ),
                     link_text: "{{#playpen my.rs editable no_run should_panic}}",
-                    }]);
+                },
+            ]
+        );
     }
 
     #[test]
     fn test_find_all_link_types() {
         let s = "Some random text with escaped playpen {{#include file.rs}} and \\{{#contents are \
-             insignifficant in escaped link}} some more\n text  {{#playpen my.rs editable no_run \
+                 insignifficant in escaped link}} some more\n text  {{#playpen my.rs editable \
-             should_panic}} ...";
+                 no_run should_panic}} ...";
 
         let res = find_links(s).collect::<Vec<_>>();
         println!("\nOUTPUT: {:?}\n", res);
         assert_eq!(res.len(), 3);
-    assert_eq!(res[0],
+        assert_eq!(
+            res[0],
             Link {
                 start_index: 38,
                 end_index: 58,
                 link: LinkType::IncludeRangeFull(PathBuf::from("file.rs"), ..),
                 link_text: "{{#include file.rs}}",
-               });
+            }
-    assert_eq!(res[1],
+        );
+        assert_eq!(
+            res[1],
             Link {
                 start_index: 63,
                 end_index: 112,
                 link: LinkType::Escaped,
                 link_text: "\\{{#contents are insignifficant in escaped link}}",
-               });
+            }
-    assert_eq!(res[2],
+        );
+        assert_eq!(
+            res[2],
             Link {
                 start_index: 130,
                 end_index: 177,
-                   link: LinkType::Playpen(PathBuf::from("my.rs"),
+                link: LinkType::Playpen(
-                                           vec!["editable", "no_run", "should_panic"]),
+                    PathBuf::from("my.rs"),
+                    vec!["editable", "no_run", "should_panic"]
+                ),
                 link_text: "{{#playpen my.rs editable no_run should_panic}}",
-               });
+            }
+        );
+    }
+
 }

tests/dummy_book/mod.rs

@@ -85,6 +85,18 @@ pub fn assert_contains_strings<P: AsRef<Path>>(filename: P, strings: &[&str]) {
     }
 }
 
+pub fn assert_doesnt_contain_strings<P: AsRef<Path>>(filename: P, strings: &[&str]) {
+    let filename = filename.as_ref();
+    let content = file_to_string(filename).expect("Couldn't read the file's contents");
+
+    for s in strings {
+        assert!(!content.contains(s),
+                "Found {:?} in {}\n\n{}",
+                s,
+                filename.display(),
+                content);
+    }
+}
 
 
 /// Recursively copy an entire directory tree to somewhere else (a la `cp -r`).

tests/dummy_book/src/SUMMARY.md

@@ -2,10 +2,11 @@
 
 [Introduction](intro.md)
 
-- [First Chapter](./first/index.md)
+- [First Chapter](first/index.md)
-    - [Nested Chapter](./first/nested.md)
+    - [Nested Chapter](first/nested.md)
-- [Second Chapter](./second.md)
+    - [Includes](first/includes.md)
+- [Second Chapter](second.md)
 
 ---
 
-[Conclusion](./conclusion.md)
+[Conclusion](conclusion.md)

tests/dummy_book/src/first/includes.md

@@ -0,0 +1,3 @@
+# Includes
+
+{{#include ../SUMMARY.md::}}

tests/rendered_output.rs

@@ -7,7 +7,7 @@ extern crate walkdir;
 
 mod dummy_book;
 
-use dummy_book::{assert_contains_strings, DummyBook};
+use dummy_book::{assert_contains_strings, assert_doesnt_contain_strings, DummyBook};
 
 use std::fs;
 use std::io::Write;
@@ -29,7 +29,7 @@ const TOC_TOP_LEVEL: &[&'static str] = &[
     "Conclusion",
     "Introduction",
 ];
-const TOC_SECOND_LEVEL: &[&'static str] = &["1.1. Nested Chapter"];
+const TOC_SECOND_LEVEL: &[&'static str] = &["1.1. Nested Chapter", "1.2. Includes"];
 
 /// Make sure you can load the dummy book and build it without panicking.
 #[test]
@@ -49,7 +49,8 @@ fn by_default_mdbook_generates_rendered_content_in_the_book_directory() {
     md.build().unwrap();
 
     assert!(temp.path().join("book").exists());
-    assert!(temp.path().join("book").join("index.html").exists());
+    let index_file = md.build_dir_for("html").join("index.html");
+    assert!(index_file.exists());
 }
 
 #[test]
@@ -281,7 +282,7 @@ fn create_missing_file_with_config() {
 /// This makes sure you can include a Rust file with `{{#playpen example.rs}}`.
 /// Specification is in `book-example/src/format/rust.md`
 #[test]
-fn able_to_include_rust_files_in_chapters() {
+fn able_to_include_playpen_files_in_chapters() {
     let temp = DummyBook::new().build().unwrap();
     let md = MDBook::load(temp.path()).unwrap();
     md.build().unwrap();
@@ -292,7 +293,24 @@ fn able_to_include_rust_files_in_chapters() {
         r#"class="playpen""#,
         r#"println!("Hello World!");"#,
     ];
-    assert_contains_strings(second, playpen_strings);
+
+    assert_contains_strings(&second, playpen_strings);
+    assert_doesnt_contain_strings(&second, &["{{#playpen example.rs}}"]);
+}
+
+/// This makes sure you can include a Rust file with `{{#include ../SUMMARY.md}}`.
+#[test]
+fn able_to_include_files_in_chapters() {
+    let temp = DummyBook::new().build().unwrap();
+    let md = MDBook::load(temp.path()).unwrap();
+    md.build().unwrap();
+
+    let includes = temp.path().join("book/first/includes.html");
+
+    let summary_strings = &["<h1>Summary</h1>", ">First Chapter</a>"];
+    assert_contains_strings(&includes, summary_strings);
+
+    assert_doesnt_contain_strings(&includes, &["{{#include ../SUMMARY.md::}}"]);
 }
 
 #[test]