🍞 Check for old / stale translations#117
Conversation
|
I'd like this feature. But I think, if we can instead to use print with line of number of the stale I18n keys will be better. - locales/en.yml:120 hello_world |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a feature to detect unused translation keys and output them to a STALE.yml file, helping translators remove obsolete entries.
- Introduces a
stalefunction ingenerator.rsthat scans existing locale files and identifies keys no longer used. - Adds a
find_stale_translationshelper to collect stale entries per locale. - Hooks the
stalecheck into the CLI (main.rs) so CI fails when stale translations are found.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/extract/src/generator.rs | Added stale and find_stale_translations functions to emit STALE.yml |
| crates/cli/src/main.rs | Invoked generator::stale before generate and handled its error flag |
| write_file(&output_path, filename, &text)?; | ||
|
|
||
| // Finally, return error for let CI fail | ||
| let err = std::io::Error::new(std::io::ErrorKind::Other, ""); |
There was a problem hiding this comment.
Providing an empty error message makes CI failures hard to debug; include a descriptive message like "Found stale translations" to clarify the failure reason.
| let err = std::io::Error::new(std::io::ErrorKind::Other, ""); | |
| let err = std::io::Error::new(std::io::ErrorKind::Other, "Stale translations detected. Please review the generated file."); |
| let output_path = output_path.as_ref().display().to_string(); | ||
|
|
||
| let ignore_file = |fname: &str| fname.ends_with(&output_filename); | ||
| let data = load_locales(&output_path, ignore_file); |
There was a problem hiding this comment.
[nitpick] This shadows the output_path parameter with a String, which can be confusing; consider renaming the local variable or performing this conversion outside of the loop.
| let output_path = output_path.as_ref().display().to_string(); | |
| let ignore_file = |fname: &str| fname.ends_with(&output_filename); | |
| let data = load_locales(&output_path, ignore_file); | |
| let output_path_str = output_path.as_ref().display().to_string(); | |
| let ignore_file = |fname: &str| fname.ends_with(&output_filename); | |
| let data = load_locales(&output_path_str, ignore_file); |
| for locale in all_locales { | ||
| println!("Checking [{}] for stale translations...", locale); | ||
|
|
||
| // ~/work/my-project/locales | ||
| let output_path = output_path.as_ref().display().to_string(); | ||
|
|
||
| let ignore_file = |fname: &str| fname.ends_with(&output_filename); | ||
| let data = load_locales(&output_path, ignore_file); |
There was a problem hiding this comment.
Converting output_path to a String on every iteration incurs unnecessary overhead; move this conversion before the for locale loop.
| for locale in all_locales { | |
| println!("Checking [{}] for stale translations...", locale); | |
| // ~/work/my-project/locales | |
| let output_path = output_path.as_ref().display().to_string(); | |
| let ignore_file = |fname: &str| fname.ends_with(&output_filename); | |
| let data = load_locales(&output_path, ignore_file); | |
| // Convert output_path to a String once before the loop | |
| let output_path_str = output_path.as_ref().display().to_string(); | |
| for locale in all_locales { | |
| println!("Checking [{}] for stale translations...", locale); | |
| let ignore_file = |fname: &str| fname.ends_with(&output_filename); | |
| let data = load_locales(&output_path_str, ignore_file); |
| /// Check for stale translation (translations that aren't used by the app anymore) | ||
| pub fn stale<'a, P: AsRef<Path>>( | ||
| output_path: P, | ||
| all_locales: &Vec<String>, |
There was a problem hiding this comment.
[nitpick] Accepting &Vec<String> ties the function to a vector; consider using a slice &[String] or a generic impl AsRef<str> for more flexibility.
| all_locales: &Vec<String>, | |
| all_locales: &[String], |
|
|
||
| let output_path = Path::new(&source_path).join(&cfg.load_path); | ||
|
|
||
| let result = generator::stale(&output_path, &cfg.available_locales, messages.clone()); |
There was a problem hiding this comment.
[nitpick] Shadowing the variable result in the same scope can reduce readability; consider using distinct names or chaining results to avoid shadowing.
I noticed in our application we had some phrases that still existed in our
app.ymlfile, but weren't used in the application anymore, causing unnecessary work for our translators. This PR adds a bit of code to detect these and write out aSTALE.ymlfile (next toTODO.yml) so they can be manually removed.