Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Upgrade to the new custom preprocessor system #2

Closed
wants to merge 4 commits into from
Closed

WIP: Upgrade to the new custom preprocessor system #2

wants to merge 4 commits into from

Conversation

Michael-F-Bryan
Copy link
Contributor

I'm mainly doing this to see what working with the rust-lang/mdBook#792 will be like. Translating from the previous "wrapped" form to the plugin system was super simple, I essentially reused the existing Mermaid preprocessor and copied the guts from the nop-preprocessor example.

Obviously this won't be ready for other people to use until the custom preprocessor PR makes its way into a release.

How does it look to you @badboy?

@badboy badboy self-requested a review September 19, 2018 14:53
@badboy
Copy link
Owner

badboy commented Sep 19, 2018

It's missing the actual implementation (so probably forgot to commit src/lib.rs?)

@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Sep 20, 2018

I didn't actually need to touch anything in src/lib.rs. The executable is essentially just a shim which will defer to the mdbook_mermaid::Mermaid preprocessor, so we reuse the existing code.

When I'm writing up a page for the user guide I should probably mention the easiest way to write a custom preprocessor is to implement the Preprocessor trait on your type in lib.rs then for the mdbook-whatever binary you can copy/paste in a program that will pass everything through to the relevant Preprocessor methods.

@badboy
Copy link
Owner

badboy commented Sep 20, 2018

Oooooh, I see. Even better. I'll check the implementation again.

@Michael-F-Bryan
Copy link
Contributor Author

@badboy I'm thinking of merging in rust-lang/mdBook#792. Did you get a chance to review this PR?

Any feedback on the general mechanism for custom preprocessors would also be useful.

@badboy
Copy link
Owner

badboy commented Oct 4, 2018

tl;dr: go for it! Yey, preprocessors!

I haven't given the upstream PR a proper look. This PR is small enough that I'm not worried about the changes.

I do like the idea of simple transformations on JSON (afaik pandoc does it similar). That will allow to have plugins in any language easily (avoids huge and slower-to-compile Rust plugins).
One other thing I have in mind is extracting the basic functionality from mdbook to parse a book (-> smaller dependency for preprocessors). I haven't looked closely into mdbook to see if that would actually make a difference though.

@Michael-F-Bryan
Copy link
Contributor Author

One other thing I have in mind is extracting the basic functionality from mdbook to parse a book (-> smaller dependency for preprocessors). I haven't looked closely into mdbook to see if that would actually make a difference though.

I'd really like to do this too! At the moment mdbook takes forever to compile due to a large number of dependencies, which in turn have their own dependencies. The core operations are actually really simple and shouldn't require much more than the serde, pulldown-cmark and toml crates.

@Michael-F-Bryan
Copy link
Contributor Author

@badboy I just pushed 0.2.2 to crates.io with proper custom preprocessor support. That should hopefully make using the mdbook-mermaid preprocessor easier and not require installing from git.

@badboy
Copy link
Owner

badboy commented Jul 9, 2019

I was nudged enough to finally update this, merged and updated here: https://github.com/badboy/mdbook-mermaid/compare/3cea267..3951f00?diff=unified

@badboy badboy closed this Jul 9, 2019
@Michael-F-Bryan Michael-F-Bryan deleted the preprocessor-plugin branch July 10, 2019 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants