diff --git a/CHANGELOG.md b/CHANGELOG.md index 367229a..b9d43ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to polygo. Versions follow [SemVer](https://semver.org); dat ## Unreleased +- A finding about a whole file (`locale`, and a `syntax` error whose parser gave no position) now sits on line 1 instead of nowhere. A review can only comment on a line, so those findings never reached the pull request that introduced them: the bot reported a pull request adding an unlisted locale as clean, which is the failure the `locale` code exists to end. + - `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. diff --git a/src/check/formats/syntax.rs b/src/check/formats/syntax.rs index 2aec8c6..178863e 100644 --- a/src/check/formats/syntax.rs +++ b/src/check/formats/syntax.rs @@ -18,6 +18,9 @@ pub struct Broken { pub idx: usize, /// Path relative to the checked root, as findings name files. pub file: String, + /// Where to put it. A finding about the file rather than a line in it goes on line 1: + /// a reviewer can only comment on a line, so a finding without one never reaches the + /// pull request that introduced it. pub line: Option, /// The locale the path stands for, or the source locale for a source file. pub locale: String, @@ -112,7 +115,7 @@ pub fn unlisted(cx: &mut crate::check::report::Cx, idx: usize) { cx.emit_at( idx, file.clone(), - None, + Some(1), &file, &locale, Code::Locale, @@ -141,7 +144,7 @@ fn parse_error(path: &Path, format: Format) -> Option<(Option, String)> { }; let e = parsed.err()?; let message = one_line(&format!("{e:#}")); - Some((line_of(&message, &text), message)) + Some((Some(line_of(&message, &text).unwrap_or(1)), message)) } /// Where the fix goes. Parsers say either "at line 584 column 2" or "at byte 34886"; a @@ -244,6 +247,6 @@ mod tests { assert_eq!(line_of("expected ',' or '}' at byte 5", text), Some(3)); assert_eq!(line_of("expected ',' at byte 0", text), Some(1)); assert_eq!(line_of("beyond the end at byte 9999", text), Some(3)); - assert_eq!(line_of("no numbers here", text), None); + assert_eq!(line_of("no numbers here", text), None); // the caller falls back to 1 } } diff --git a/tests/check_syntax.rs b/tests/check_syntax.rs index f653f2d..70a32f1 100644 --- a/tests/check_syntax.rs +++ b/tests/check_syntax.rs @@ -232,3 +232,39 @@ fn an_android_qualifier_directory_is_not_a_locale() { "night and w600dp are qualifiers, not languages: {out}" ); } + +/// A finding about a whole file has to sit on a line, or the reviewer cannot say it: a +/// pull request that adds an unlisted locale would come back clean, which is the very +/// failure the `locale` code exists to end. +#[test] +fn a_file_level_finding_lands_on_line_one_so_a_review_can_carry_it() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + project(root); + fs::write( + root.join("locales/it.json"), + "{\n \"a\": \"Ciao {{name}}\",\n \"b\": \"Arrivederci\"\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(); + // The diff that adds the file, as GitHub would give it. + fs::write( + root.join("pr.diff"), + "--- a/locales/it.json\n+++ b/locales/it.json\n@@ -0,0 +1,4 @@\n+{\n+ \"a\": \"Ciao {{name}}\",\n+ \"b\": \"Arrivederci\"\n+}\n", + ) + .unwrap(); + let (_, out, _) = check(root, &["--review", "--diff", "pr.diff"]); + let v: serde_json::Value = serde_json::from_str(&out).unwrap(); + let c = &v["comments"][0]; + assert_eq!(c["path"], "locales/it.json", "{out}"); + assert_eq!(c["line"], 1); + assert!( + c["body"].as_str().unwrap().contains("Locale not checked"), + "{}", + c["body"] + ); +}