rust-lang / mdBook

Create book from markdown files. Like Gitbook but implemented in Rust
https://rust-lang.github.io/mdBook/
Mozilla Public License 2.0
17.69k stars 1.61k forks source link

Use of non-rust preprocessors isn't clearly documented in the guide book #1462

Closed shonfeder closed 3 years ago

shonfeder commented 3 years ago

After reading the chapter on preprocessors and learning that they were meant to operate in "typical unix fashion", I assumed I could just drop any stream transformer as a preprocessors. Come to find out, the executable must also respond to the subcommand supports <foo>, where <foo> is some format.

My initial reaction is that this is not typical unix fashion, and that it requires an unnecessarily tight coupling between the preprocessor and mdBook. I wonder if it would be enough just to leave the designation of support up to the renderer key, rather than double checking with the tool whether it really can support what we've configured it to support :)

However, at minimum, adding this requirement into the docs should help future users.

Thanks for a very nice tool!

shonfeder commented 3 years ago

I am trying to test what I think should be a dead simple no-op preprocessor as a bash script:

#!/usr/bin/env bash
set -euo pipefail

jq -M -c <&0

I have saved this as test.sh and am hooking into the default init config thus:

[book]
authors = ["Me"]
language = "en"
multilingual = false
src = "src"
title = "testbook"

[preprocessor.emojify]

command = "./test.sh"

And even tho I've confirmed that the script is just emitting the exact same json it reads in (and I've tried other permutations, like just writing out line by line etc), attempts to mdbook build fail with

$ mdbook build
2021-02-12 21:27:27 [INFO] (mdbook::book): Book building has started
2021-02-12 21:27:28 [ERROR] (mdbook::utils): Error: Unable to parse the preprocessed book
2021-02-12 21:27:28 [ERROR] (mdbook::utils):    Caused By: invalid type: map, expected a sequence at line 1 column 1

Any idea what I may be doing wrong?

shonfeder commented 3 years ago

Of course, I also tried this even simpler pass through first:

#!/usr/bin/env bash
set -euo pipefail

cat <&0

I only turned to jq to ensure that what I was emitting was valid json.

tulir commented 3 years ago

It seems to give preprocessors an array of [config, book data], but expects only the book data in return. I haven't managed to actually modify the content though.

ehuss commented 3 years ago

I wrote a minimal preprocessor in Python at https://github.com/rust-lang/mdBook/issues/975#issuecomment-509821512 (not sure if that is still up-to-date).

Usually most implementations are written in Rust which can rely on the library here to take care of most of the work. It would be great to improve the documentation.

I don't know why the design uses the "supports" arg instead of just passing that as a key in the JSON. It might be worthwhile to rethink the API, but I suspect that would be a lot of work to ensure a good design. Changing it may result in a lot of churn for existing plugins, so I wouldn't want to do it lightly. (And unfortunately I don't have a lot of time to work on mdbook.)

shonfeder commented 3 years ago

Very helpful. Thank you both for helping me puzzle this out! So a pretty minimal no-op bash preprocessor is

#!/usr/bin/env bash
set -euo pipefail

jq -M -c .[1] <&0

Just returning the second item in the array.

pauliyobo commented 3 years ago

Hello. Would a PR with documentation changes be considered? Maybe a section that explains how MdBook shares informations with the preprocessor would help? Additionally should an example in a non rust language, such as python which has been used before in these issues be included as well?

Dylan-DPC-zz commented 3 years ago

@pauliyobo

Yeah sure go ahead and submit a PR, we'll take the discussion from there.

almereyda commented 3 years ago

It feels this was closed prematurely, due to the modal phrase "should fix" present in the PR description, now merged by @ehuss.

Why I am curios to know, if the people present here would also still consider the special case of the bash preprocessor (and with it the possibility for more UNIX-oid streaming parsers) be a useful addition to the documentation, or even a supported option of mdBook?

If yes, we should reopen here.

shonfeder commented 3 years ago

I'd consider #1480 to satisfy this issue, since it's about the documentation.

But I also find the current API for preprocessesors weird and cumbersome, and I think changing it (or augmenting it) to expect normal unix filters (i.e., without calling the thing twice, once just to check it the specified filter actually will filter what you've said it will filter) would be a nice improvement.

That said, I've figured things out for our needs, and I'm not very invested one way or another.

Thanks for checking in!