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

Migrate to rinja #2487

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

Main argument for switching to rinja: compilation fails if your template is invalid. It also generates very performant code (as can be seen here).

Some extra advantages: it makes the code simpler to follow. Storing everything into a JSON map made it pretty tricky to understand what was going on and needed to have a lot more checks in the code.

With this JSON map gone, I also simplified code to handle chapter kinds: it's now a Chapter enum, making it easier to understand.

Last point: rustdoc, docs.rs and clippy lints page already switched to rinja so I think it tells a lot about how reliable rinja is. :)

Considering how big the PR is, please don't hesitate to ask me if anything is unclear.

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Nov 18, 2024
@notriddle
Copy link
Contributor

In the current version of mdBook, mdbook init --theme drops copies of the hbs files that book authors can then modify, allowing them to change the HTML without recompiling.

This PR seems to remove that functionality. I don't think that's a good idea without first taking stock of customization that popular books use, helping them migrate over to some better way(s) to accomplish the same things, and only then deprecating and removing support for custom hbs.

In any case, you really need to mention stuff like that in the PR description.

@GuillaumeGomez
Copy link
Member Author

I didn't realize it was a feature. I saw a test checking that but didn't check further, should have. Gonna investigate.

@GuillaumeGomez
Copy link
Member Author

@notriddle
Copy link
Contributor

notriddle commented Nov 19, 2024

rust-by-example has a custom language picker, and zjp-cn has a custom TOC.

#2490 adds their own logo.

@GuillaumeGomez
Copy link
Member Author

The custom TOC is actually to add something that was missing at the time: TOC for headings in the current page. For rust-by-example it could be switched to JS.

So in here, I think we'll need to discuss more th @ehuss to see if adding options --html-before-content and --html-after-content to allow users to add their own HTML without modifying templates is acceptable, and if adding a --html-header-content to add their own content in <head> is ok. It could be a major release etc.

On one hand, it would make mdBook closer to rustdoc in this regard.

But in any case, I think the code simplification and having template errors at compile-time is a big plus.

Waiting to hear from @ehuss then. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants