implementing preprocessors #532

JaimeValdemoros · 2018-01-07T15:29:37Z

No description provided.

Michael-F-Bryan · 2018-01-07T16:29:24Z

src/preprocess/links.rs

+        for section in &mut book.sections {
+            match *section {
+                BookItem::Chapter(ref mut ch) => {
+                    let content = ::std::mem::replace(&mut ch.content, String::new());


I don't think you need to use mem::replace() here. You could store the replaced text in a temporary (let replaced = replace_all(&ch.content, base)) then update ch.content on the next line.

Good point, thanks

Michael-F-Bryan · 2018-01-07T23:44:17Z

@JaimeValdemoros I just had another skim through what you've done so far and it looks pretty good 👍

You may want to add a couple tests for the preprocessor discovery, because I feel like there are a couple edge cases we haven't accounted for. For example, what happens if the user passes in an unknown preprocessor, if you specify a preprocessor twice, or if one of the elements in the preprocessor array is actually a table?

The easiest way to write the test is probably to create a bunch of dummy configs (just raw strings), run Config::from_str() to load it, then run determine_preprocessors() and make sure we got the preprocessors we expected. You may also want to add a name() method to the Preprocessor trait so we can log something like "running the \"{}\" preprocessor", and it also helps figure out which preprocessors were found while testing.

Michael-F-Bryan · 2018-01-14T09:34:16Z

Hi @JaimeValdemoros, this is just a friendly ping to see how you're going with everything 😀 How far through this issue would you say you are? And is there anything I can do to help?

JaimeValdemoros · 2018-01-14T16:46:57Z

Hey - sorry about the delay, I've been away and haven't been able to get a free evening to get it finished. Will have it done tomorrow afternoon if that's ok. On 14 Jan 2018 10:34 am, "Michael Bryan" <notifications@github.com> wrote: Hi @JaimeValdemoros <https://github.com/jaimevaldemoros>, this is just a friendly ping to see how you're going with everything 😀 How far through this issue would you say you are? And is there anything I can do to help? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#532 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH7W0_BHAkA06LRP6Oqx3GL52QNGy6X9ks5tKcoZgaJpZM4RVrx6> .

JaimeValdemoros · 2018-01-15T22:58:44Z

@Michael-F-Bryan I've made a couple of small changes that felt a bit more sensible:

Moved the "preprocess" field under the "build" header rather than standalone (as an optional field)
Report an error if the preprocessor isn't recognised
Allow explicitly disabling the default option by setting an empty list
Added a 'name' field to the Preprocessor trait, matching the Renderer trait
Added some tests

Is there anything you think is still missing?

Michael-F-Bryan

Overall I'm really happy with this PR, it looks clean and well thought-out and easily testable 👍

I've made a couple comments/questions, but they're mainly to throw around ideas or ask your opinion.

We may also want to add an integration test which makes sure the preprocessors actually get called.

This could be as simple as creating a dummy Preprocessor which contains a Rc<AtomicBool> and will toggle the bool when it gets called. The test then does a build of the dummy book and asserts the bool is now true.

Michael-F-Bryan · 2018-01-16T09:19:51Z

src/book/mod.rs

+        Some(ref p) => p,
+        // If no preprocessor field is set, default to the LinkPreprocessor. This allows you
+        // to disable the LinkPreprocessor by setting "preprocess" to an empty list.
+        None => return Ok(vec![Box::new(LinkPreprocessor::new())])


I reckon we may want to pull this bit out into its own default_preprocessors() function, where we immediately return vec![Box::new(LinkPreprocessor::new())]. I imagine that'll make things easier to update in the future, and it's immediately obvious what the default preprocessors are.

Done - you're right, makes it a bit cleaner

Michael-F-Bryan · 2018-01-16T09:23:00Z

src/book/mod.rs

-                _ => {}
-            }
+    for key in preprocess_list {
+        if key == "links" {


What are your thoughts on turning this if key == "links" {...} else { bail!() } into a match statement? Now that preprocessors are easier to work with, I imagine we'll be adding a bunch more in the future.

Michael-F-Bryan · 2018-01-16T09:24:24Z

src/book/mod.rs

@@ -149,14 +154,23 @@ impl MDBook {
    pub fn build(&self) -> Result<()> {
        debug!("[fn]: build");

+        let mut preprocessed_book = self.book.clone();


I like that we're not mutating the original book here and instead preprocessing its clone 👍

Michael-F-Bryan · 2018-01-16T09:28:22Z

src/book/mod.rs

@@ -190,6 +204,13 @@ impl MDBook {
        self
    }

+    /// You can add a new preprocessor by using this method.
+    /// The only requirement is for your renderer to implement the Preprocessor trait.


This doc-comment should probably be reworded in the "imperative" tense to describe what it does. So something like "Register a [Preprocessor] to be used when rendering the book", with the word "Preprocessor" being a link to the relevant trait so people can see the required methods and what types already implement Preprocessor.

Michael-F-Bryan · 2018-01-16T09:30:27Z

src/preprocess/mod.rs

+
+pub trait Preprocessor {
+    fn name(&self) -> &str;
+    fn run(&self, ctx: &PreprocessorContext, book: &mut Book) -> Result<()>;


I'm tossing up whether to include the Book as part of the PreprocessorContext. Do you think the book to be processed should be part of the context or should we keep it as a second argument?

I think it's helpful to distinguish the fact that you only expect the Book itself to be mutated, and that the PreprocessorContext is additional information it can use to help in that mutation. Having a single parameter &mut PreprocessorContext obscures that slightly and has fewer guarantees about what the Preprocessor is allowed to do to the context.

Ah yeah that's a good point. I completely forgot the Book is &mut while we pass an immutable reference for the PreprocessorContext.

Michael-F-Bryan · 2018-01-16T09:32:50Z

src/preprocess/mod.rs

+use std::path::PathBuf;
+
+pub struct PreprocessorContext {
+    pub src_dir: PathBuf


It's probably better for PreprocessorContext to contain a reference (or clone, if you don't want to deal with lifetimes) to the Config and the book's root. That way a preprocessor can hook into book.toml for configuration, and the src_dir can be discovered by combining config.book.src and the book root.

So something like this:

pub struct PreprocessorContext { pub config: Config, pub root: PathBuf, // maybe also include the `Book` here... I dunno }

That makes sense, given the other fields of the MDBook shouldn't be needed by the preprocessor. I've made it a clone for now since a borrow would be problematic here:

https://github.com/rust-lang-nursery/mdBook/blob/bf093e2f5f0e8c7f455b6e021cb72adb974836d4/src/book/mod.rs#L205

since we'd have a reference to an inner field of self in the Context and additionally trying to take a reference to itself to pass into iter.

Michael-F-Bryan · 2018-01-16T15:57:31Z

@JaimeValdemoros, I've made a temporary fix (#549) to CI so travis won't fail and erroneously say that this PR is broken. If you want, you can git rebase on top of the latest master to pick up the fix and make CI work normally for this PR.

…t struct

JaimeValdemoros · 2018-01-16T22:40:20Z

Thanks @Michael-F-Bryan - this is the first time I've rebased onto a fork so hopefully I've done it right

Michael-F-Bryan · 2018-01-16T23:48:20Z

tests/testing.rs

+fn mdbook_runs_preprocessors() {
+
+    lazy_static! {
+        static ref HAS_RUN: Mutex<bool> = Mutex::new(false);


I don't think we need to use lazy_static!() here, an Arc<Mutex<bool>> should be enough.

You're right, I was trying to have a dummy option in the Config and have the book add the DummyPreprocessor to the list when it found the dummy option, which did require the lazy_static since the test wasn't constructing it directly. I should've rethought the design when I switched to using with_preprocessor and building the DummyPreprocessor directly.

Michael-F-Bryan · 2018-01-16T23:50:33Z

this is the first time I've rebased onto a fork so hopefully I've done it right

Looks like everything's okay 👍

JaimeValdemoros · 2018-01-17T19:10:23Z

Just to list what I've done since the last checklist:

Factor out explicit default_preprocessors method
Match on preprocessor options instead of nested if's
Test to make sure preprocessors are being run
Use rustdoc link to Preprocessor trait (this worked on my machine but the Renderer link didn't work as it stood on my machine so probably needs checking to see if my machine does something odd)
Log to debug as preprocessors are being run

Michael-F-Bryan · 2018-01-17T23:17:26Z

I just had another look through the code and I'm happy with it. I think I'll merge this now and then we can do a follow-up PR to document how preprocessors work in the user guide.

Awesome work @JaimeValdemoros, I'm really happy with this!

…564) * Looks like we forgot to recursively apply replace_all() in #532 * Removed some print statements * Made sure we ignore the rendered dummy_book

…eprocessors implementing preprocessors

…ust-lang#564) * Looks like we forgot to recursively apply replace_all() in rust-lang#532 * Removed some print statements * Made sure we ignore the rendered dummy_book

JaimeValdemoros mentioned this pull request Jan 7, 2018

Introduce Preprocessors #530

Closed

Michael-F-Bryan reviewed Jan 7, 2018

View reviewed changes

Michael-F-Bryan mentioned this pull request Jan 13, 2018

Mdbook should translate internal references #408

Closed

Michael-F-Bryan suggested changes Jan 16, 2018

View reviewed changes

Michael-F-Bryan mentioned this pull request Jan 16, 2018

Support in-repo links, with verification and output-dependent conversion #431

Open

JaimeValdemoros added 14 commits January 16, 2018 22:39

Initial Preprocessor trait implementation

01df904

Factor out replace_all preprocessor

cad76a9

Start determining preprocessors

9668110

Remove unnecessary mem::replace

f282a55

Add pre-processing step to build method of MDBook

12815fe

Preprocess links by default

4cc708e

Change name of link preprocessor

144358b

Add LinkPreprocessor::new constructor

9c922cf

Clean up LinkPreprocessor exports and use explicit PreprocessorContex…

b98ed3f

…t struct

Revert reordering

08027b8

Move preprocess field location and add tests

b599956

Add test to make sure pre-processors are being run

4177288

Pull out default preprocessors into function

f2d7b70

Turn chained if's into match

90fa1b4

JaimeValdemoros force-pushed the implementing-preprocessors branch from 535890c to 90fa1b4 Compare January 16, 2018 22:39

Update comment with rustdoc link

47cc571

Michael-F-Bryan reviewed Jan 16, 2018

View reviewed changes

Make Preprocessor context store config and root

0d62578

JaimeValdemoros added 2 commits January 17, 2018 19:02

Use Arc instead of lazy_static for dummy preprocessor

80a20eb

Log as each preprocessor is run

1136f67

Michael-F-Bryan approved these changes Jan 17, 2018

View reviewed changes

Michael-F-Bryan changed the title ~~WIP implementing preprocessors~~ implementing preprocessors Jan 17, 2018

Michael-F-Bryan merged commit 7b4b70a into rust-lang:master Jan 17, 2018

sorin-davidoi mentioned this pull request Jan 19, 2018

0.1 Release Prerequisites #558

Closed

3 tasks

Michael-F-Bryan added a commit to Michael-F-Bryan/mdBook that referenced this pull request Jan 21, 2018

Looks like we forgot to recursively apply replace_all() in rust-lang#532

b272b17

Michael-F-Bryan mentioned this pull request Jan 21, 2018

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

Merged

Michael-F-Bryan mentioned this pull request Jan 27, 2018

Translate relative links to other .md files during rendering (fixes #588) #589

Closed

Michael-F-Bryan mentioned this pull request Feb 24, 2018

Next stage in the plugin architecture #631

Open

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020

Merge pull request rust-lang#532 from JaimeValdemoros/implementing-pr…

ae48595

…eprocessors implementing preprocessors

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implementing preprocessors #532

implementing preprocessors #532

JaimeValdemoros commented Jan 7, 2018

Michael-F-Bryan Jan 7, 2018

JaimeValdemoros Jan 7, 2018

Michael-F-Bryan commented Jan 7, 2018

Michael-F-Bryan commented Jan 14, 2018

JaimeValdemoros commented Jan 14, 2018 via email

JaimeValdemoros commented Jan 15, 2018

Michael-F-Bryan left a comment

Michael-F-Bryan Jan 16, 2018

JaimeValdemoros Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

JaimeValdemoros Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

JaimeValdemoros Jan 17, 2018 •

edited

Loading

Michael-F-Bryan commented Jan 16, 2018

JaimeValdemoros commented Jan 16, 2018

Michael-F-Bryan Jan 16, 2018

JaimeValdemoros Jan 17, 2018

Michael-F-Bryan commented Jan 16, 2018

JaimeValdemoros commented Jan 17, 2018

Michael-F-Bryan commented Jan 17, 2018

implementing preprocessors #532

implementing preprocessors #532

Conversation

JaimeValdemoros commented Jan 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Jan 7, 2018

Michael-F-Bryan commented Jan 14, 2018

JaimeValdemoros commented Jan 14, 2018 via email

JaimeValdemoros commented Jan 15, 2018

Michael-F-Bryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaimeValdemoros Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Michael-F-Bryan commented Jan 16, 2018

JaimeValdemoros commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Jan 16, 2018

JaimeValdemoros commented Jan 17, 2018

Michael-F-Bryan commented Jan 17, 2018

JaimeValdemoros Jan 17, 2018 •

edited

Loading