Configurable preprocessor (#658)

* The preprocessor trait now returns a modified book instead of editing in place

* A preprocessor is told which render it's running for

* Made sure preprocessors get their renderer's name

* Users can now manually specify whether a preprocessor should run for a renderer

* You can normally use default preprocessors by default

* Got my logic around the wrong way

* Fixed the `build.use-default-preprocessors` flag

examples/de-emphasize.rs

@@ -14,45 +14,7 @@ use std::env::{args, args_os};
 use std::ffi::OsString;
 use std::process;
 
-struct Deemphasize;
-
-impl Preprocessor for Deemphasize {
-    fn name(&self) -> &str {
-        "md-links-to-html-links"
-    }
-
-    fn run(&self, _ctx: &PreprocessorContext, book: &mut Book) -> Result<()> {
-        eprintln!("Running '{}' preprocessor", self.name());
-        let mut res: Option<_> = None;
-        let mut num_removed_items = 0;
-        book.for_each_mut(|item: &mut BookItem| {
-            if let Some(Err(_)) = res {
-                return;
-            }
-            if let BookItem::Chapter(ref mut chapter) = *item {
-                eprintln!("{}: processing chapter '{}'", self.name(), chapter.name);
-                res = Some(
-                    match Deemphasize::remove_emphasis(&mut num_removed_items, chapter) {
-                        Ok(md) => {
-                            chapter.content = md;
-                            Ok(())
-                        }
-                        Err(err) => Err(err),
-                    },
-                );
-            }
-        });
-        eprintln!(
-            "{}: removed {} events from markdown stream.",
-            self.name(),
-            num_removed_items
-        );
-        match res {
-            Some(res) => res,
-            None => Ok(()),
-        }
-    }
-}
+const NAME: &str = "md-links-to-html-links";
 
 fn do_it(book: OsString) -> Result<()> {
     let mut book = MDBook::load(book)?;
@@ -71,24 +33,66 @@ fn main() {
     }
 }
 
-impl Deemphasize {
-    fn remove_emphasis(num_removed_items: &mut i32, chapter: &mut Chapter) -> Result<String> {
-        let mut buf = String::with_capacity(chapter.content.len());
-        let events = Parser::new(&chapter.content).filter(|e| {
-            let should_keep = match *e {
-                Event::Start(Tag::Emphasis)
-                | Event::Start(Tag::Strong)
-                | Event::End(Tag::Emphasis)
-                | Event::End(Tag::Strong) => false,
-                _ => true,
-            };
-            if !should_keep {
-                *num_removed_items += 1;
-            }
-            should_keep
-        });
-        cmark(events, &mut buf, None)
-            .map(|_| buf)
-            .map_err(|err| Error::from(format!("Markdown serialization failed: {}", err)))
+struct Deemphasize;
+
+impl Preprocessor for Deemphasize {
+    fn name(&self) -> &str {
+        NAME
+    }
+
+    fn run(&self, _ctx: &PreprocessorContext, mut book: Book) -> Result<Book> {
+        eprintln!("Running '{}' preprocessor", self.name());
+        let mut num_removed_items = 0;
+
+        process(&mut book.sections, &mut num_removed_items)?;
+
+        eprintln!(
+            "{}: removed {} events from markdown stream.",
+            self.name(),
+            num_removed_items
+        );
+
+        Ok(book)
     }
 }
+
+fn process<'a, I>(items: I, num_removed_items: &mut usize) -> Result<()>
+where
+    I: IntoIterator<Item = &'a mut BookItem> + 'a,
+{
+    for item in items {
+        if let BookItem::Chapter(ref mut chapter) = *item {
+            eprintln!("{}: processing chapter '{}'", NAME, chapter.name);
+
+            let md = remove_emphasis(num_removed_items, chapter)?;
+            chapter.content = md;
+        }
+    }
+
+    Ok(())
+}
+
+fn remove_emphasis(
+    num_removed_items: &mut usize,
+    chapter: &mut Chapter,
+) -> Result<String> {
+    let mut buf = String::with_capacity(chapter.content.len());
+
+    let events = Parser::new(&chapter.content).filter(|e| {
+        let should_keep = match *e {
+            Event::Start(Tag::Emphasis)
+            | Event::Start(Tag::Strong)
+            | Event::End(Tag::Emphasis)
+            | Event::End(Tag::Strong) => false,
+            _ => true,
+        };
+        if !should_keep {
+            *num_removed_items += 1;
+        }
+        should_keep
+    });
+
+    cmark(events, &mut buf, None).map(|_| buf).map_err(|err| {
+        Error::from(format!("Markdown serialization failed: {}", err))
+    })
+}

src/book/mod.rs

@@ -149,23 +149,39 @@ impl MDBook {
     pub fn build(&self) -> Result<()> {
         info!("Book building has started");
 
-        let mut preprocessed_book = self.book.clone();
-        let preprocess_ctx = PreprocessorContext::new(self.root.clone(), self.config.clone());
-
-        for preprocessor in &self.preprocessors {
-            debug!("Running the {} preprocessor.", preprocessor.name());
-            preprocessor.run(&preprocess_ctx, &mut preprocessed_book)?;
-        }
-
         for renderer in &self.renderers {
-            info!("Running the {} backend", renderer.name());
-            self.run_renderer(&preprocessed_book, renderer.as_ref())?;
+            self.execute_build_process(&**renderer)?;
         }
 
         Ok(())
     }
 
-    fn run_renderer(&self, preprocessed_book: &Book, renderer: &Renderer) -> Result<()> {
+    /// Run the entire build process for a particular `Renderer`.
+    fn execute_build_process(&self, renderer: &Renderer) -> Result<()> {
+        let mut preprocessed_book = self.book.clone();
+        let preprocess_ctx = PreprocessorContext::new(self.root.clone(),
+                                     self.config.clone(),
+                                     renderer.name().to_string());
+
+        for preprocessor in &self.preprocessors {
+            if preprocessor_should_run(&**preprocessor, renderer, &self.config) {
+                debug!("Running the {} preprocessor.", preprocessor.name());
+                preprocessed_book =
+                    preprocessor.run(&preprocess_ctx, preprocessed_book)?;
+            }
+        }
+
+        info!("Running the {} backend", renderer.name());
+        self.render(&preprocessed_book, renderer)?;
+
+        Ok(())
+    }
+
+    fn render(
+        &self,
+        preprocessed_book: &Book,
+        renderer: &Renderer,
+    ) -> Result<()> {
         let name = renderer.name();
         let build_dir = self.build_dir_for(name);
         if build_dir.exists() {
@@ -215,13 +231,16 @@ impl MDBook {
 
         let temp_dir = TempFileBuilder::new().prefix("mdbook-").tempdir()?;
 
-        let preprocess_context = PreprocessorContext::new(self.root.clone(), self.config.clone());
+        // FIXME: Is "test" the proper renderer name to use here?
+        let preprocess_context = PreprocessorContext::new(self.root.clone(),
+                                                          self.config.clone(),
+                                                          "test".to_string());
 
-        LinkPreprocessor::new().run(&preprocess_context, &mut self.book)?;
+        let book = LinkPreprocessor::new().run(&preprocess_context, self.book.clone())?;
         // Index Preprocessor is disabled so that chapter paths continue to point to the
         // actual markdown files.
 
-        for item in self.iter() {
+        for item in book.iter() {
             if let BookItem::Chapter(ref ch) = *item {
                 if !ch.path.as_os_str().is_empty() {
                     let path = self.source_dir().join(&ch.path);
@@ -330,19 +349,32 @@ fn default_preprocessors() -> Vec<Box<Preprocessor>> {
     ]
 }
 
+fn is_default_preprocessor(pre: &Preprocessor) -> bool {
+    let name = pre.name();
+    name == LinkPreprocessor::NAME || name == IndexPreprocessor::NAME
+}
+
 /// Look at the `MDBook` and try to figure out what preprocessors to run.
 fn determine_preprocessors(config: &Config) -> Result<Vec<Box<Preprocessor>>> {
-    let preprocess_list = match config.build.preprocess {
-        Some(ref p) => p,
+    let preprocessor_keys = config.get("preprocessor")
+        .and_then(|value| value.as_table())
+        .map(|table| table.keys());
+
+    let mut preprocessors = if config.build.use_default_preprocessors {
+        default_preprocessors()
+    } else {
+        Vec::new()
+    };
+
+    let preprocessor_keys = match preprocessor_keys {
+        Some(keys) => keys,
         // If no preprocessor field is set, default to the LinkPreprocessor and
         // IndexPreprocessor. This allows you to disable default preprocessors
         // by setting "preprocess" to an empty list.
-        None => return Ok(default_preprocessors()),
+        None => return Ok(preprocessors),
     };
 
-    let mut preprocessors: Vec<Box<Preprocessor>> = Vec::new();
-
-    for key in preprocess_list {
+    for key in preprocessor_keys {
         match key.as_ref() {
             "links" => preprocessors.push(Box::new(LinkPreprocessor::new())),
             "index" => preprocessors.push(Box::new(IndexPreprocessor::new())),
@@ -366,6 +398,31 @@ fn interpret_custom_renderer(key: &str, table: &Value) -> Box<Renderer> {
     Box::new(CmdRenderer::new(key.to_string(), command.to_string()))
 }
 
+/// Check whether we should run a particular `Preprocessor` in combination
+/// with the renderer, falling back to `Preprocessor::supports_renderer()`
+/// method if the user doesn't say anything.
+///
+/// The `build.use-default-preprocessors` config option can be used to ensure
+/// default preprocessors always run if they support the renderer.
+fn preprocessor_should_run(preprocessor: &Preprocessor, renderer: &Renderer, cfg: &Config) -> bool {
+    // default preprocessors should be run by default (if supported)
+    if cfg.build.use_default_preprocessors && is_default_preprocessor(preprocessor) {
+        return preprocessor.supports_renderer(renderer.name());
+    }
+
+    let key = format!("preprocessor.{}.renderers", preprocessor.name());
+    let renderer_name = renderer.name();
+
+    if let Some(Value::Array(ref explicit_renderers)) = cfg.get(&key) {
+        return explicit_renderers.into_iter()
+            .filter_map(|val| val.as_str())
+            .any(|name| name == renderer_name);
+    }
+
+    preprocessor.supports_renderer(renderer_name)
+}
+
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -413,8 +470,8 @@ mod tests {
     fn config_defaults_to_link_and_index_preprocessor_if_not_set() {
         let cfg = Config::default();
 
-        // make sure we haven't got anything in the `output` table
-        assert!(cfg.build.preprocess.is_none());
+        // make sure we haven't got anything in the `preprocessor` table
+        assert!(cfg.get("preprocessor").is_none());
 
         let got = determine_preprocessors(&cfg);
 
@@ -425,26 +482,13 @@ mod tests {
     }
 
     #[test]
-    fn config_doesnt_default_if_empty() {
-        let cfg_str: &'static str = r#"
-        [book]
-        title = "Some Book"
+    fn use_default_preprocessors_works() {
+        let mut cfg = Config::default();
+        cfg.build.use_default_preprocessors = false;
 
-        [build]
-        build-dir = "outputs"
-        create-missing = false
-        preprocess = []
-        "#;
+        let got = determine_preprocessors(&cfg).unwrap();
 
-        let cfg = Config::from_str(cfg_str).unwrap();
-
-        // make sure we have something in the `output` table
-        assert!(cfg.build.preprocess.is_some());
-
-        let got = determine_preprocessors(&cfg);
-
-        assert!(got.is_ok());
-        assert!(got.unwrap().is_empty());
+        assert_eq!(got.len(), 0);
     }
 
     #[test]
@@ -453,19 +497,73 @@ mod tests {
         [book]
         title = "Some Book"
 
+        [preprocessor.random]
+
         [build]
         build-dir = "outputs"
         create-missing = false
-        preprocess = ["random"]
         "#;
 
         let cfg = Config::from_str(cfg_str).unwrap();
 
-        // make sure we have something in the `output` table
-        assert!(cfg.build.preprocess.is_some());
+        // make sure the `preprocessor.random` table exists
+        assert!(cfg.get_preprocessor("random").is_some());
 
         let got = determine_preprocessors(&cfg);
 
         assert!(got.is_err());
     }
+
+    #[test]
+    fn config_respects_preprocessor_selection() {
+        let cfg_str: &'static str = r#"
+        [preprocessor.links]
+        renderers = ["html"]
+        "#;
+
+        let cfg = Config::from_str(cfg_str).unwrap();
+
+        // double-check that we can access preprocessor.links.renderers[0]
+        let html = cfg.get_preprocessor("links")
+            .and_then(|links| links.get("renderers"))
+            .and_then(|renderers| renderers.as_array())
+            .and_then(|renderers| renderers.get(0))
+            .and_then(|renderer| renderer.as_str())
+            .unwrap();
+        assert_eq!(html, "html");
+        let html_renderer = HtmlHandlebars::default();
+        let pre = LinkPreprocessor::new();
+
+        let should_run = preprocessor_should_run(&pre, &html_renderer, &cfg);
+        assert!(should_run);
+    }
+
+    struct BoolPreprocessor(bool);
+    impl Preprocessor for BoolPreprocessor {
+        fn name(&self) -> &str {
+            "bool-preprocessor"
+        }
+
+        fn run(&self, _ctx: &PreprocessorContext, _book: Book) -> Result<Book> {
+            unimplemented!()
+        }
+
+        fn supports_renderer(&self, _renderer: &str) -> bool {
+            self.0
+        }
+    }
+
+    #[test]
+    fn preprocessor_should_run_falls_back_to_supports_renderer_method() {
+        let cfg = Config::default();
+        let html = HtmlHandlebars::new();
+
+        let should_be = true;
+        let got = preprocessor_should_run(&BoolPreprocessor(should_be), &html, &cfg);
+        assert_eq!(got, should_be);
+
+        let should_be = false;
+        let got = preprocessor_should_run(&BoolPreprocessor(should_be), &html, &cfg);
+        assert_eq!(got, should_be);
+    }
 }

src/config.rs

@@ -209,6 +209,18 @@ impl Config {
         Ok(())
     }
 
+    /// Get the table associated with a particular renderer.
+    pub fn get_renderer<I: AsRef<str>>(&self, index: I) -> Option<&Table> {
+        let key = format!("output.{}", index.as_ref());
+        self.get(&key).and_then(|v| v.as_table())
+    }
+
+    /// Get the table associated with a particular preprocessor.
+    pub fn get_preprocessor<I: AsRef<str>>(&self, index: I) -> Option<&Table> {
+        let key = format!("preprocessor.{}", index.as_ref());
+        self.get(&key).and_then(|v| v.as_table())
+    }
+
     fn from_legacy(mut table: Value) -> Config {
         let mut cfg = Config::default();
 
@@ -382,8 +394,9 @@ pub struct BuildConfig {
     /// Should non-existent markdown files specified in `SETTINGS.md` be created
     /// if they don't exist?
     pub create_missing: bool,
-    /// Which preprocessors should be applied
-    pub preprocess: Option<Vec<String>>,
+    /// Should the default preprocessors always be used when they are
+    /// compatible with the renderer?
+    pub use_default_preprocessors: bool,
 }
 
 impl Default for BuildConfig {
@@ -391,7 +404,7 @@ impl Default for BuildConfig {
         BuildConfig {
             build_dir: PathBuf::from("book"),
             create_missing: true,
-            preprocess: None,
+            use_default_preprocessors: true,
         }
     }
 }
@@ -551,7 +564,7 @@ mod tests {
         [build]
         build-dir = "outputs"
         create-missing = false
-        preprocess = ["first_preprocessor", "second_preprocessor"]
+        use-default-preprocessors = true
 
         [output.html]
         theme = "./themedir"
@@ -562,6 +575,10 @@ mod tests {
         [output.html.playpen]
         editable = true
         editor = "ace"
+
+        [preprocess.first]
+
+        [preprocess.second]
         "#;
 
     #[test]
@@ -579,10 +596,7 @@ mod tests {
         let build_should_be = BuildConfig {
             build_dir: PathBuf::from("outputs"),
             create_missing: false,
-            preprocess: Some(vec![
-                "first_preprocessor".to_string(),
-                "second_preprocessor".to_string(),
-            ]),
+            use_default_preprocessors: true,
         };
         let playpen_should_be = Playpen {
             editable: true,
@@ -684,7 +698,7 @@ mod tests {
         let build_should_be = BuildConfig {
             build_dir: PathBuf::from("my-book"),
             create_missing: true,
-            preprocess: None,
+            use_default_preprocessors: true,
         };
 
         let html_should_be = HtmlConfig {

src/preprocess/index.rs

@@ -11,6 +11,8 @@ use book::{Book, BookItem};
 pub struct IndexPreprocessor;
 
 impl IndexPreprocessor {
+    pub(crate) const NAME: &'static str = "index";
+
     /// Create a new `IndexPreprocessor`.
     pub fn new() -> Self {
         IndexPreprocessor
@@ -19,10 +21,10 @@ impl IndexPreprocessor {
 
 impl Preprocessor for IndexPreprocessor {
     fn name(&self) -> &str {
-        "index"
+        Self::NAME
     }
 
-    fn run(&self, ctx: &PreprocessorContext, book: &mut Book) -> Result<()> {
+    fn run(&self, ctx: &PreprocessorContext, mut book: Book) -> Result<Book> {
         let source_dir = ctx.root.join(&ctx.config.book.src);
         book.for_each_mut(|section: &mut BookItem| {
             if let BookItem::Chapter(ref mut ch) = *section {
@@ -37,7 +39,7 @@ impl Preprocessor for IndexPreprocessor {
             }
         });
 
-        Ok(())
+        Ok(book)
     }
 }
 

src/preprocess/links.rs

@@ -16,6 +16,8 @@ const MAX_LINK_NESTED_DEPTH: usize = 10;
 pub struct LinkPreprocessor;
 
 impl LinkPreprocessor {
+    pub(crate) const NAME: &'static str = "links";
+
     /// Create a new `LinkPreprocessor`.
     pub fn new() -> Self {
         LinkPreprocessor
@@ -24,10 +26,10 @@ impl LinkPreprocessor {
 
 impl Preprocessor for LinkPreprocessor {
     fn name(&self) -> &str {
-        "links"
+        Self::NAME
     }
 
-    fn run(&self, ctx: &PreprocessorContext, book: &mut Book) -> Result<()> {
+    fn run(&self, ctx: &PreprocessorContext, mut book: Book) -> Result<Book> {
         let src_dir = ctx.root.join(&ctx.config.book.src);
 
         book.for_each_mut(|section: &mut BookItem| {
@@ -43,7 +45,7 @@ impl Preprocessor for LinkPreprocessor {
             }
         });
 
-        Ok(())
+        Ok(book)
     }
 }
 

src/preprocess/mod.rs

@@ -19,12 +19,14 @@ pub struct PreprocessorContext {
     pub root: PathBuf,
     /// The book configuration (`book.toml`).
     pub config: Config,
+    /// The `Renderer` this preprocessor is being used with.
+    pub renderer: String,
 }
 
 impl PreprocessorContext {
     /// Create a new `PreprocessorContext`.
-    pub(crate) fn new(root: PathBuf, config: Config) -> Self {
-        PreprocessorContext { root, config }
+    pub(crate) fn new(root: PathBuf, config: Config, renderer: String) -> Self {
+        PreprocessorContext { root, config, renderer }
     }
 }
 
@@ -36,5 +38,13 @@ pub trait Preprocessor {
 
     /// Run this `Preprocessor`, allowing it to update the book before it is
     /// given to a renderer.
-    fn run(&self, ctx: &PreprocessorContext, book: &mut Book) -> Result<()>;
+    fn run(&self, ctx: &PreprocessorContext, book: Book) -> Result<Book>;
+
+    /// A hint to `MDBook` whether this preprocessor is compatible with a
+    /// particular renderer.
+    ///
+    /// By default, always returns `true`.
+    fn supports_renderer(&self, _renderer: &str) -> bool {
+        true
+    }
 }

tests/build_process.rs

@@ -0,0 +1,80 @@
+extern crate mdbook;
+
+mod dummy_book;
+
+use dummy_book::DummyBook;
+use mdbook::book::Book;
+use mdbook::config::Config;
+use mdbook::errors::*;
+use mdbook::preprocess::{Preprocessor, PreprocessorContext};
+use mdbook::renderer::{RenderContext, Renderer};
+use mdbook::MDBook;
+use std::sync::{Arc, Mutex};
+
+struct Spy(Arc<Mutex<Inner>>);
+
+#[derive(Debug, Default)]
+struct Inner {
+    run_count: usize,
+    rendered_with: Vec<String>,
+}
+
+impl Preprocessor for Spy {
+    fn name(&self) -> &str {
+        "dummy"
+    }
+
+    fn run(&self, ctx: &PreprocessorContext, book: Book) -> Result<Book> {
+        let mut inner = self.0.lock().unwrap();
+        inner.run_count += 1;
+        inner.rendered_with.push(ctx.renderer.clone());
+        Ok(book)
+    }
+}
+
+impl Renderer for Spy {
+    fn name(&self) -> &str {
+        "dummy"
+    }
+
+    fn render(&self, _ctx: &RenderContext) -> Result<()> {
+        let mut inner = self.0.lock().unwrap();
+        inner.run_count += 1;
+        Ok(())
+    }
+}
+
+#[test]
+fn mdbook_runs_preprocessors() {
+    let spy: Arc<Mutex<Inner>> = Default::default();
+
+    let temp = DummyBook::new().build().unwrap();
+    let cfg = Config::default();
+
+    let mut book = MDBook::load_with_config(temp.path(), cfg).unwrap();
+    book.with_preprecessor(Spy(Arc::clone(&spy)));
+    book.build().unwrap();
+
+    let inner = spy.lock().unwrap();
+    assert_eq!(inner.run_count, 1);
+    assert_eq!(inner.rendered_with.len(), 1);
+    assert_eq!(
+        "html", inner.rendered_with[0],
+        "We should have been run with the default HTML renderer"
+    );
+}
+
+#[test]
+fn mdbook_runs_renderers() {
+    let spy: Arc<Mutex<Inner>> = Default::default();
+
+    let temp = DummyBook::new().build().unwrap();
+    let cfg = Config::default();
+
+    let mut book = MDBook::load_with_config(temp.path(), cfg).unwrap();
+    book.with_renderer(Spy(Arc::clone(&spy)));
+    book.build().unwrap();
+
+    let inner = spy.lock().unwrap();
+    assert_eq!(inner.run_count, 1);
+}

tests/testing.rs

@@ -4,14 +4,8 @@ mod dummy_book;
 
 use dummy_book::DummyBook;
 
-use mdbook::book::Book;
-use mdbook::config::Config;
-use mdbook::errors::*;
-use mdbook::preprocess::{Preprocessor, PreprocessorContext};
 use mdbook::MDBook;
 
-use std::sync::{Arc, Mutex};
-
 #[test]
 fn mdbook_can_correctly_test_a_passing_book() {
     let temp = DummyBook::new().with_passing_test(true).build().unwrap();
@@ -27,30 +21,3 @@ fn mdbook_detects_book_with_failing_tests() {
 
     assert!(md.test(vec![]).is_err());
 }
-
-#[test]
-fn mdbook_runs_preprocessors() {
-    let has_run: Arc<Mutex<bool>> = Arc::new(Mutex::new(false));
-
-    struct DummyPreprocessor(Arc<Mutex<bool>>);
-
-    impl Preprocessor for DummyPreprocessor {
-        fn name(&self) -> &str {
-            "dummy"
-        }
-
-        fn run(&self, _ctx: &PreprocessorContext, _book: &mut Book) -> Result<()> {
-            *self.0.lock().unwrap() = true;
-            Ok(())
-        }
-    }
-
-    let temp = DummyBook::new().build().unwrap();
-    let cfg = Config::default();
-
-    let mut book = MDBook::load_with_config(temp.path(), cfg).unwrap();
-    book.with_preprecessor(DummyPreprocessor(Arc::clone(&has_run)));
-    book.build().unwrap();
-
-    assert!(*has_run.lock().unwrap())
-}