Add mdbook 0.5.x Support#62
Conversation
Migrate mdbook-quiz to support mdbook 0.5.x, dropping 0.4.x support. Changes: 1. Extract internal utilities (replaces mdbook-preprocessor-utils): - utils/asset.rs: Asset struct and asset_generator! macro - utils/html.rs: HtmlElementBuilder for HTML generation - utils/preprocessor.rs: SimplePreprocessor trait and driver 2. Update dependencies: - Remove: mdbook 0.4.45, mdbook-preprocessor-utils 0.2.0 - Add: mdbook-preprocessor 0.5, mdbook-core 0.5 - Add: serde, rayon, env_logger, log, chrono, semver 3. Migrate to mdbook 0.5.x APIs: - Use mdbook_preprocessor instead of mdbook::preprocess - Replace config.get_preprocessor() with config.get() - Handle Result<bool> in supports_renderer() - Update Book::sections to Book::items 4. Update build script: - Inline copy_assets() implementation - Make JS build optional with better error handling 5. Temporarily disable test (needs mdbook 0.5 test harness) BREAKING CHANGE: Drops support for mdbook 0.4.x Bump version: 0.4.0 → 0.5.0 Fixes cognitive-engineering-lab#61
Update CI workflow to download and use mdbook v0.5.2 instead of v0.4.45. Ensures CI tests run against the correct mdbook version matching the updated dependencies.
Add crates/mdbook-quiz/js/*.{js,css,wasm,map} to .gitignore.
These files are generated by the depot build process and should
not be committed to the repository.
- Add tempfile dev-dependency for test isolation - Implement local TestHarness to replace deprecated MdbookTestHarness - Re-enable test_quiz_generator test with mdbook 0.5.x APIs - Uses SimplePreprocessorDriver and manual Book construction Completes the test migration TODO from commit 3de0657
There was a problem hiding this comment.
Pull request overview
This PR migrates mdbook-quiz from mdbook 0.4.45 to mdbook 0.5.x, addressing the deprecation of the mdbook-preprocessor-utils library by extracting and implementing the required utility functionality directly within the crate.
Key changes:
- Migrated from
mdbook 0.4.45tomdbook-preprocessor 0.5andmdbook-core 0.5 - Extracted internal utilities (
SimplePreprocessor,HtmlElementBuilder,Asset) from deprecatedmdbook-preprocessor-utilsinto newutilsmodule - Updated API usage for mdbook 0.5.x compatibility (config access, Book.items iterator, Result-based renderer support)
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mdbook-quiz/src/utils/preprocessor.rs | New file implementing SimplePreprocessor trait and driver for mdbook 0.5.x, replacing functionality from deprecated mdbook-preprocessor-utils |
| crates/mdbook-quiz/src/utils/mod.rs | Module exports for the new utils package |
| crates/mdbook-quiz/src/utils/html.rs | HTML element builder utility extracted from deprecated library |
| crates/mdbook-quiz/src/utils/asset.rs | Asset handling and macro generation extracted from deprecated library |
| crates/mdbook-quiz/src/main.rs | Updated imports, config parsing for mdbook 0.5.x API, and custom test harness implementation |
| crates/mdbook-quiz/build.rs | Inlined copy_assets implementation with improved error handling and graceful fallback |
| crates/mdbook-quiz/Cargo.toml | Version bump to 0.5.0 and dependency updates for mdbook 0.5.x |
| Cargo.lock | Dependency tree updates reflecting new mdbook-preprocessor and mdbook-core packages |
| .gitignore | Added generated JS/CSS assets to ignore list |
| .github/workflows/setup/action.yaml | Updated CI to use mdbook v0.5.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| for entry in src_entries { | ||
| let path = entry?.path(); | ||
| fs::copy(&path, dst_dir.join(path.file_name().unwrap()))?; |
There was a problem hiding this comment.
Calling unwrap() on path.file_name() will panic if the path ends with ".." or if it's empty. While this is unlikely given the input comes from fs::read_dir, it's safer to handle this case explicitly with proper error handling using the ? operator with context, or at minimum, using expect() with a descriptive message.
| fs::copy(&path, dst_dir.join(path.file_name().unwrap()))?; | |
| let file_name = path | |
| .file_name() | |
| .ok_or_else(|| Error::msg(format!("Path has no file name: {}", path.display())))?; | |
| fs::copy(&path, dst_dir.join(file_name))?; |
| for asset in self.sp.linked_assets() { | ||
| let asset_rel = prefix.join(P::name()).join(asset.name); | ||
| let asset_str = asset_rel.display().to_string(); | ||
| let link = match &*asset_rel.extension().unwrap().to_string_lossy() { |
There was a problem hiding this comment.
Calling unwrap() on asset_rel.extension() will panic if an asset file has no extension. While the current FRONTEND_ASSETS only include files with extensions (.js, .css), this creates a fragile dependency. If an asset without an extension is added in the future, this code will panic at runtime. Consider using if let or match to handle the None case gracefully, or use unwrap_or to provide a default behavior.
| let link = match &*asset_rel.extension().unwrap().to_string_lossy() { | |
| let ext = match asset_rel.extension() { | |
| Some(ext) => ext.to_string_lossy(), | |
| None => continue, | |
| }; | |
| let link = match &*ext { |
|
Sorry for the Copilot spam. Apparently my GitHub had some experimental flag enabled that made it review any change I made, even in public repositories. My sincere apologies, and have already disabled that functionality. |
|
Btw thanks for implementing this, it's on my backlog to review it, will try to get it checked and merged within a week or two. |
|
@willcrichton, Is there any movement on this PR? I appreciate this is unpaid work :D |
|
Sorry, just been swamped at work. If this is blocking, I would use @yan-pi's fork until I get the changes merged in. |
No worries, I understand and can relate Thanks for a great plugin. |
|
Thanks for this PR! We're writing a book with mdbook 0.5 and need this fix. Tried installing from this branch but hit the depot build dependency for the JS assets — cargo install fails without pre-built quiz-embed.iife.js and style.css. Would it be possible to either commit the built JS assets to this branch or document how to build them without depot (e.g. with pnpm/vite)? Happy to help if there's a path forward. |
|
As a workaround, you can run: That should proxy Depot's build step. |
|
Tested this PR against our mdbook 0.5 book (padamson/t2t). Built from the branch using This PR replaces the existing |
This should Closes #61
Summary
This PR adds support for mdbook 0.5.x to mdbook-quiz, migrating away from mdbook 0.4.x. The migration includes extracting internal utilities to replace the deprecated
mdbook-preprocessor-utils, updating to the new mdbook crate structure, and ensuring all tests pass with the new APIs.Changes
This PR consists of 4 commits:
feat: migrate to mdbook 0.5.x(3de0657):Core Migration:
mdbook 0.4.45dependency withmdbook-preprocessor 0.5andmdbook-core 0.5mdbook-preprocessor-utils 0.2.0Extract Internal Utilities:
Since
mdbook-preprocessor-utilsis not available for mdbook 0.5.x, this commit extracts the required utilities into the crate:API Migrations:
mdbook_preprocessorinstead ofmdbook::preprocessconfig.get_preprocessor()withconfig.get()Result<bool>return type insupports_renderer()Book::sectionstoBook::itemsiteratorBuild Script Updates:
copy_assets()implementationVersion Bump: 0.4.0 → 0.5.0
Breaking Change: Drops support for mdbook 0.4.x
ci: update mdbook to v0.5.2(5ce4017):chore: ignore generated JS/CSS assets(16d5c5a):.gitignorecrates/mdbook-quiz/js/*.{js,css,wasm,map}(These files are generated by the depot build process and should not be committed to the repository.)test: implement test harness for mdbook 0.5.x(5afb0c6):Test Migration:
Since
MdbookTestHarnessfrommdbook-preprocessor-utilsis no longer available, this commit implements a localTestHarnessfor testing the preprocessor with mdbook 0.5.x.Testing:
The test validates end-to-end quiz preprocessing including:
{{#quiz}})All tests pass with mdbook 0.5.2:
No changes to quiz files required - The quiz TOML schema remains backward compatible
Disclaimer
I've used AI to analyze the new API from mdBook, focusing on the mdbook-quiz dependencies and repository structure. I've manually tested and compiled the floresta-docs, which I'm currently reading; I'm not a maintainer of the project. I have also coded most of the PR myself. I feel this disclaimer is necessary because I genuinely want to contribute without bothering the maintainers of the project.