From 572ac6718bc35ec21406aa8343ae881a009cb10e Mon Sep 17 00:00:00 2001 From: Nas <36938454+Na5co@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:59:24 +0300 Subject: [PATCH] check: a locale file nobody listed is not silently ignored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A repository adds locales/it.json and forgets to put `it` in target_locales: polygo checked nothing in it and said the project was fine — a translator's whole language, unlooked at. The same shape as the unparsable file, and found the same way: a test fixture did exactly this and the reviewer reported a clean pull request. `locale` (warning) names the file and what to do. Only real language tags count, so a README.json or an Android values-night is not mistaken for a locale, and a file that does not parse keeps its syntax error rather than being reported twice for the same cause. Corpus fingerprint unchanged: detection lists every locale it finds, so a project without polygo.toml has nothing to warn about. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 + README.md | 1 + src/check/code.rs | 9 +++- src/check/formats/mod.rs | 1 + src/check/formats/syntax.rs | 43 ++++++++++++++++++ tests/check_syntax.rs | 90 +++++++++++++++++++++++++++++++++++++ 6 files changed, 145 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adc2634..367229a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to polygo. Versions follow [SemVer](https://semver.org); dat ## Unreleased +- `locale` (warning): a locale file sitting next to the ones being checked whose locale is not in `target_locales` — a translator's whole language, silently unchecked. Found the same way as the `syntax` gap: a test fixture added `locales/it.json`, forgot to list `it`, and the reviewer reported a clean pull request. Only real language tags count, so a `README.json` or an Android `values-night` is not mistaken for a locale, and a file that does not parse keeps its `syntax` error instead of being reported twice. + - `polygo-bot` runs on Cloud Run, where its always-free tier covers roughly 18,000 pull requests a month: `bot/deploy-cloud-run.sh` takes a project and the App's key and does the rest (APIs, Secret Manager, Cloud Build, deploy), then prints the webhook URL to paste into the App. `POLYGO_BOT_SYNC=1` reviews before answering GitHub, since Cloud Run stops a container's CPU once the response goes out and a background thread would be frozen mid-review. - The bot refuses what an App anyone can install will eventually be pointed at: a repository over `POLYGO_BOT_MAX_REPO_MB` (default 500, asked of the API before anything is fetched) and an installation asking for more than `POLYGO_BOT_MAX_REVIEWS` reviews in ten minutes (default 20). diff --git a/README.md b/README.md index fe561cf..cc9dcb1 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,7 @@ Ten seconds, no setup. It reads the string files your app already has and report | code | severity | what it catches | |---|:-:|---| +| `locale` | warning | a locale file sitting next to the others that nothing checks, because its locale is not in `target_locales` | | `syntax` | error | a string file that does not parse (a dropped comma, an unclosed ``) — reported instead of the locale silently vanishing from the run | | `placeholders` | error | `%1$@`, `{{name}}`, `%(count)s`, `{0}`, `{n, plural, …}` missing, added, retyped or reordered — and a translated *name* (`{{ modelli }}` for `{{ models }}`) named as such | | `plural` | error | a CLDR form the locale needs and doesn't have: `few`/`many` for Polish, six for Arabic; ICU, `.xcstrings`, `.stringsdict`, ``, `msgstr[n]`, i18next `_few` | diff --git a/src/check/code.rs b/src/check/code.rs index 99ceca1..842fc7e 100644 --- a/src/check/code.rs +++ b/src/check/code.rs @@ -30,6 +30,7 @@ impl std::fmt::Display for Severity { #[serde(rename_all = "lowercase")] pub enum Code { Syntax, + Locale, Placeholders, Plural, Markup, @@ -55,8 +56,9 @@ pub enum Code { } impl Code { - pub const ALL: [Code; 23] = [ + pub const ALL: [Code; 24] = [ Code::Syntax, + Code::Locale, Code::Placeholders, Code::Plural, Code::Markup, @@ -84,6 +86,7 @@ impl Code { pub fn as_str(self) -> &'static str { match self { Code::Syntax => "syntax", + Code::Locale => "locale", Code::Placeholders => "placeholders", Code::Plural => "plural", Code::Markup => "markup", @@ -142,6 +145,7 @@ impl Code { pub fn title(self) -> &'static str { match self { Code::Syntax => "Unparsable file", + Code::Locale => "Locale not checked", Code::Placeholders => "Placeholder mismatch", Code::Plural => "Missing plural form", Code::Markup => "Markup mismatch", @@ -173,6 +177,9 @@ impl Code { Code::Syntax => { "A string file cannot be parsed: a comma missing from a JSON catalog, an unclosed , a .po entry without its msgstr. Reported with the line the parser stopped at. It is an error on its own because of what it hides: a file polygo cannot read has no keys, so every other check would find nothing wrong with it and the locale would quietly vanish from the run." } + Code::Locale => { + "A locale file sits next to the ones being checked, but its locale is not in target_locales, so nothing in it is looked at: a translator's whole language, silently unchecked. Add the locale to polygo.toml (or `polygo locales add`), or delete the file. Only names that are real language tags count, so a README.json or an Android values-night directory is not mistaken for a locale." + } Code::Placeholders => { "A format placeholder (%1$@, {{name}}, %(count)s, {0}, {n, plural, …}) is missing, added, retyped or reordered in the translation. At runtime the value lands in the wrong place, shows raw, or crashes (Android: %d where %s was expected). A placeholder whose name was translated ({{ models }} → {{ modelli }}, %(count)s → %(anzahl)s) is the most common placeholder bug of all; it is named as such, and `polygo check --fix` puts the source name back with no model involved. Unnumbered printf arguments are numbered by position, so reordering them needs explicit %2$s %1$s. In zero/one/two plural forms the count may be left out where those are exact counts; not where `one` also covers 21, 31 (ru, uk, be, hr, sr, bs, lt, lv)." } diff --git a/src/check/formats/mod.rs b/src/check/formats/mod.rs index e893cc6..4e52284 100644 --- a/src/check/formats/mod.rs +++ b/src/check/formats/mod.rs @@ -24,6 +24,7 @@ pub fn check(cx: &mut Cx) -> Result<()> { Format::Arb | Format::Po | Format::Resx => {} } files::check(cx, idx)?; + syntax::unlisted(cx, idx); } Ok(()) } diff --git a/src/check/formats/syntax.rs b/src/check/formats/syntax.rs index 6a45678..2aec8c6 100644 --- a/src/check/formats/syntax.rs +++ b/src/check/formats/syntax.rs @@ -1,3 +1,6 @@ +//! Files the project has but does not read: one that cannot be parsed, and one whose locale +//! nobody listed. +//! //! A string file that does not parse. Without this pass a locale file someone broke — a //! comma dropped from a JSON catalog, an unclosed `` — is simply not there: the //! detector cannot read it, so the locale disappears from the run and `check` says the @@ -81,6 +84,46 @@ pub fn check(cx: &mut crate::check::report::Cx, broken: &[Broken]) -> bool { fatal } +/// A locale file the project is not checking, because its locale is not in +/// `target_locales`. Someone added the language and forgot to say so, and everything in it +/// — every placeholder, every plural — goes unlooked at. +pub fn unlisted(cx: &mut crate::check::report::Cx, idx: usize) { + let (root, cfg) = (cx.root.to_path_buf(), cx.cfg); + let spec = &cfg.files[idx]; + let source = cfg.source_locale.clone(); + let targets = cfg.target_locales.clone(); + let found: Vec<(String, PathBuf)> = locale_files(&root, spec) + .into_iter() + .filter(|(locale, path)| { + locale != &source + && !targets.contains(locale) + && crate::init::is_locale(locale) + // A file that does not parse is already reported as such, and that is why + // the detector left its locale out; saying so twice helps nobody. + && parse_error(path, spec.format).is_none() + }) + .collect(); + for (locale, path) in found { + let file = path + .strip_prefix(&root) + .unwrap_or(&path) + .to_string_lossy() + .replace('\\', "/"); + cx.emit_at( + idx, + file.clone(), + None, + &file, + &locale, + Code::Locale, + Severity::Warning, + format!( + "nothing in this file is checked: `{locale}` is not in target_locales (`polygo locales add {locale}`, or delete the file)" + ), + ); + } +} + /// `(line, message)` when the file does not parse, `None` when it does. fn parse_error(path: &Path, format: Format) -> Option<(Option, String)> { let text = match formats::read_text(path) { diff --git a/tests/check_syntax.rs b/tests/check_syntax.rs index 7ad1473..f653f2d 100644 --- a/tests/check_syntax.rs +++ b/tests/check_syntax.rs @@ -142,3 +142,93 @@ fn the_reviewer_comments_on_the_broken_line() { c["body"] ); } + +#[test] +fn a_locale_file_nobody_listed_is_not_silently_ignored() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + project(root); + // Someone added Italian and forgot to say so. Everything in it — every placeholder, + // every plural — would go unchecked without a word. + fs::write( + root.join("locales/it.json"), + "{\n \"a\": \"Ciao {{utente}}\",\n \"b\": \"Arrivederci\"\n}\n", + ) + .unwrap(); + // A file that matches the template but is not a locale must not be mistaken for one. + fs::write( + root.join("locales/README.json"), + "{\n \"x\": \"notes\"\n}\n", + ) + .unwrap(); + fs::write( + root.join("polygo.toml"), + "source_locale = \"en\"\ntarget_locales = [\"de\"]\n\n[[files]]\nformat = \"json\"\npath = \"locales/en.json\"\nlocale_path = \"locales/{locale}.json\"\n\n[provider]\nkind = \"mock\"\n", + ) + .unwrap(); + let (code, out, _) = check(root, &["--json"]); + assert_eq!( + code, 0, + "an unlisted locale is a warning, not an error: {out}" + ); + let v: serde_json::Value = serde_json::from_str(&out).unwrap(); + let f: Vec<&serde_json::Value> = v["findings"] + .as_array() + .unwrap() + .iter() + .filter(|f| f["code"] == "locale") + .collect(); + assert_eq!(f.len(), 1, "{out}"); + assert_eq!(f[0]["locale"], "it"); + assert_eq!(f[0]["file"], "locales/it.json"); + assert_eq!(f[0]["severity"], "warning"); + assert!( + f[0]["message"] + .as_str() + .unwrap() + .contains("`it` is not in target_locales"), + "{}", + f[0]["message"] + ); + // Listing it makes the warning go and the real bug in it appear. + fs::write( + root.join("polygo.toml"), + "source_locale = \"en\"\ntarget_locales = [\"de\", \"it\"]\n\n[[files]]\nformat = \"json\"\npath = \"locales/en.json\"\nlocale_path = \"locales/{locale}.json\"\n\n[provider]\nkind = \"mock\"\n", + ) + .unwrap(); + let (code, out, _) = check(root, &[]); + assert_eq!(code, 1, "{out}"); + assert!(!out.contains("locale:"), "{out}"); + assert!(out.contains("placeholder name translated"), "{out}"); +} + +#[test] +fn an_android_qualifier_directory_is_not_a_locale() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + for d in ["values", "values-de", "values-night", "values-w600dp"] { + fs::create_dir_all(root.join("res").join(d)).unwrap(); + fs::write( + root.join("res").join(d).join("strings.xml"), + "Hi\n", + ) + .unwrap(); + } + fs::write( + root.join("polygo.toml"), + "source_locale = \"en\"\ntarget_locales = [\"de\"]\n\n[[files]]\nformat = \"android\"\npath = \"res/values/strings.xml\"\nlocale_path = \"res/values-{android_locale}/strings.xml\"\n\n[provider]\nkind = \"mock\"\n", + ) + .unwrap(); + let (_, out, _) = check(root, &["--json"]); + let v: serde_json::Value = serde_json::from_str(&out).unwrap(); + let locales: Vec<&serde_json::Value> = v["findings"] + .as_array() + .unwrap() + .iter() + .filter(|f| f["code"] == "locale") + .collect(); + assert!( + locales.is_empty(), + "night and w600dp are qualifiers, not languages: {out}" + ); +}