-
Notifications
You must be signed in to change notification settings - Fork 36
Add book metadata to gettext extraction #283
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
==========================================
+ Coverage 84.74% 85.52% +0.77%
==========================================
Files 15 17 +2
Lines 3377 3379 +2
Branches 3377 3379 +2
==========================================
+ Hits 2862 2890 +28
+ Misses 413 387 -26
Partials 102 102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mgeisler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If translating the title and description in the .po file doesn't actually do anything, then we should not add this. Or we should rework it in some way to make it have the desired effect 😄
i18n-helpers/USAGE.md
Outdated
| > `book.description` from `book.toml`. After translating them, you can override | ||
| > the metadata per locale by setting `MDBOOK_BOOK__TITLE` and | ||
| > `MDBOOK_BOOK__DESCRIPTION` (or editing your localized `book.toml`) when | ||
| > running `mdbook build`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this makes me think that translating the string (in the .po file) doesn't actually have any effect?
|
@mgeisler I'm re-working the Implementation on this. |
|
Thanks for the detailed feedback. I've updated the changes so the translated metadata is actually used during the build:
running 145 tests test result: ok. 145 passed; 0 failed; 0 ignored; 0 measured; 0 filtered Let me know what you think. |
| Some(script) | ||
| } | ||
|
|
||
| fn inject_metadata_script(book: &mut Book, translations: &MetadataTranslations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I don't think this is a good approach: injecting JavaScript into every Markdown file (if I read it correctly) is very invasive and surprising.
People will have questions and I don't have answers for them 😄
Basically, if this is not supported in upstream mdbook, then we cannot support it here either.
Okay, so I think I understand now that this isn't supported by /// Run this `Preprocessor`, allowing it to update the book before it is
/// given to a renderer.
fn run(&self, ctx: &PreprocessorContext, book: Book) -> Result<Book>;Basically, the signature doesn't allow us to modify the If that is correct, then a first step would be to open an issue about this in the upstream Now that I have a fuller picture, I'm also okay with your first approach: extract the metadata and include instructions in a README for how to use it. With the upstream issue, the README should link back to |
Summary
Rationale
Changes
Fixes #269