Build and update script for translations #268
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
==========================================
- Coverage 84.74% 82.64% -2.11%
==========================================
Files 15 18 +3
Lines 3377 3809 +432
Branches 3377 3809 +432
==========================================
+ Hits 2862 3148 +286
- Misses 413 558 +145
- Partials 102 103 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
michael-kerscher
left a comment
michael-kerscher
left a comment
There was a problem hiding this comment.
Hi Jonathan. Thank you for this PR, this looks great and useful!
The CI piplines spotted some issues that need to be fixed.
I also gave it a first pass and have some small comments:
I noticed, that there are a lot of path "Strings" that from my perspective would benefit in being a Path. This resolves a lot of issues with Windows vs. Linux (directory separators...) and has good ergonomics that don't require a lot of string formatting :)
Regarding argument parsing, we already use clap in this repository that removes the burden of parsing and verifying the command line arguments from your shoulders :)
For an example in this repository you can have a look at https://github.com/google/mdbook-i18n-helpers/blob/main/i18n-report/src/main.rs#L53-L69 - this example also uses Paths and defaults.
| } | ||
| } | ||
|
|
||
| fn run_helper(args: &[String]) -> () { |
There was a problem hiding this comment.
We could leverage clap for parsing the arguments here
| Ok(()) | ||
| } | ||
|
|
||
| fn build(dir: &str) -> Result<(), Error>{ |
There was a problem hiding this comment.
dir could be a Path. That type has some nice operators and this resolves the issue in line 136 with manual path separators (Windows is not so keen on '/' ;))
| use zip::write::SimpleFileOptions; | ||
| use std::fs::File; | ||
|
|
||
| fn build_translation(locale: String, dest_dir: String) -> Result<(), Error>{ |
There was a problem hiding this comment.
May I ask you to split up that function into smaller functions to better spot the individual steps of the build?
| if locale == "en" { | ||
| println!("::group::Building English course"); | ||
| } else { | ||
| let locale_ref = &locale; |
There was a problem hiding this comment.
We don't need a separate variable for this -- &locale is shorter and more clear to me :)
| use zip::write::SimpleFileOptions; | ||
| use std::fs::File; | ||
|
|
||
| fn build_translation(locale: String, dest_dir: String) -> Result<(), Error>{ |
There was a problem hiding this comment.
Functions very rarely need to take an owned String as an argument. Normally you want your functions to take &str instead.
The reason is that this gives the caller more flexibility: they can do both
let s = String::from("this is a heap-allocated owned string");
build_translation(&s);and
build_translation("static non-heap allocated string");When you take a String in your function signature, you force the caller to allocate memory for it.
| pot_creation_date = Some(now.format("%Y-%m-%dT%H:%M:%S").to_string()); | ||
| } | ||
|
|
||
| println!("::group::Building {locale_ref} translation as of {:?}", pot_creation_date.clone().unwrap()); |
There was a problem hiding this comment.
Every unwrap() is a potential for a crash, so you should avoid them in production code.
Here, it looks like you initialize pot_creation_date to Some(...) in any case? If so, the type should not be Option<String> but just String. The unwrap here is basically you working around the type being inconvenient for you.
What you can do, is to initialize it like you do in the loop, and then afterwards use Option::unwrap_or_else to get a default value with chrono::Local::now().
The result would be a String, not an Option<String>.
|
|
||
| Command::new("git").args(["restore","--source", &result_str, "src/", "third_party/"]).output()?; | ||
|
|
||
| env::set_var("MDBOOK_BOOK__LANGUAGE", locale_ref); |
There was a problem hiding this comment.
This sets the environment variable for the xtask binary, right?
Can you instead set the environment variables for the Command created below? That way we avoid the global state.
| let output = Command::new("git").args(["rev-list", "-n", "1", "--before", &pot_creation_date.unwrap(), "@"]).output().unwrap(); | ||
| let result_str= String::from_utf8(output.stdout).unwrap().replace("\n", ""); | ||
|
|
||
| Command::new("git").args(["restore","--source", &result_str, "src/", "third_party/"]).output()?; |
There was a problem hiding this comment.
This line needs to take google/comprehensive-rust#2658 into account 😄
In general, please extract the paths to restore to a const at the global level. That way we can maintain it better.
There was a problem hiding this comment.
I tried Command::new("git").args(["restore","--source", &result_str, "src/", "third_party/"]).output()?; and even when I run git restore --source "$(git rev-list -n 1 --before "$pot_creation_date" @)" src/ third_party/ book.toml manually, pandoc fails and the comprehensive-rust pdf isn't created.
I just get something like:
~/comprehensive-rust$ mdbook build -d book/fa
2025-07-15 22:29:25 [INFO] (mdbook::book): Book building has started
2025-07-15 22:29:26 [INFO] (mdbook::book): Running the exerciser backend
2025-07-15 22:29:26 [INFO] (mdbook::renderer): Invoking the "exerciser" renderer
2025-07-15 22:29:27 [INFO] (mdbook::book): Running the html backend
2025-07-15 22:29:28 [INFO] (mdbook::book): Running the linkcheck backend
2025-07-15 22:29:28 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2025-07-15 22:29:28 [INFO] (mdbook::book): Running the pandoc backend
2025-07-15 22:29:28 [INFO] (mdbook::renderer): Invoking the "pandoc" renderer
2025-07-15 22:29:29 [INFO] (mdbook_pandoc::pandoc::renderer): Running pandoc
Error producing PDF.
! ==> Fatal error occurred, no output PDF file produced!
pandoc exited unsuccessfully
2025-07-15 22:29:42 [ERROR] (mdbook::renderer): Renderer exited with non-zero return code.
2025-07-15 22:29:42 [ERROR] (mdbook::utils): Error: Rendering failed
2025-07-15 22:29:42 [ERROR] (mdbook::utils): Caused By: The "pandoc" renderer failed
Could it be some sort of mismatch between the restored book.toml and the recent update to Rust 2024 edition?
mgeisler
changed the title
4 participants
Adds a script with build and update sub-commands
Fixes #18.