diff --git a/README.md b/README.md index cace98c..601ee0e 100644 --- a/README.md +++ b/README.md @@ -9,8 +9,8 @@ via `scripts/crowdin_sync.py`. `bible_loop.po`, ...). - `crowdin/crowdin.yml` — Crowdin CLI config for project 257. - `scripts/crowdin_sync.py` — CI entrypoint. Pulls current source strings - from Crowdin (default mode, via `scripts/update_strings.py`) or pushes new - local strings to Crowdin (`--push`). Runs on Bitrise; see the script's + from Crowdin (default mode, via `scripts/update_strings.py`) or pushes + local string changes to Crowdin (`--push`). Runs on Bitrise; see the script's docstring for the three modes. - `scripts/update_strings.py` — downloads a Crowdin bundle (`CROWDIN_BUNDLE_ID`) and installs its English `.po` files into @@ -18,16 +18,17 @@ via `scripts/crowdin_sync.py`. scripts/update_strings.py`) for local testing; requires a bundle scoped to these files with "include source language" enabled (there is no source-only download in crowdin-cli 4.12.0 — see the script's docstring). -- `scripts/crowdin_validator.py` — CI gate that fails a PR if it edits or - deletes an existing string, or introduces a key that collides with one in - another file. Needs no Crowdin credentials. +- `scripts/crowdin_validator.py` — CI gate on PRs touching `strings/en/*.po`. + Reports every added, edited, deleted and renamed string, and fails only on + a mistake: removing a whole `.po` file, a key that duplicates one in + another file, a key defined twice in the same file, an unparseable entry, + or an empty value. Needs no Crowdin credentials. -## Adding a new source string +## Changing source strings -Existing strings can only be edited or deleted via the Crowdin UI. To add a -brand-new one, edit the target `.po` file directly under `strings/en/` -(`app.po`, `felt_needs.po`, etc.) and append a new entry in the same style as -the rest of the file: +Add, edit and delete English strings by editing the target `.po` file +directly under `strings/en/` (`app.po`, `felt_needs.po`, etc.), in the same +style as the rest of the file: ``` msgid "new_key" @@ -35,8 +36,31 @@ msgstr "New English text" ``` Then commit and open a normal PR. `scripts/crowdin_validator.py` runs in CI -on that PR and fails if it edits or deletes an existing entry, or introduces -a key that collides with one already in another file — this needs no -Crowdin credentials. The next `scripts/crowdin_sync.py --push` run pushes the -new entries to Crowdin as an unconditional first step, ahead of its normal -pull. +on that PR (no Crowdin credentials needed) and prints what changed. It fails +the build only on a mistake — removing a whole `.po` file, a key that +duplicates one already in another file, the same key defined twice in one +file, an entry it cannot parse, or an entry with an empty value. The next +`scripts/crowdin_sync.py --push` run propagates all of it to Crowdin. + +What each kind of change costs on the Crowdin side, since the validator +reports but does not block them: + +| Change | Effect on existing translations | +| --- | --- | +| Add a `msgid` | None — the new string starts untranslated. | +| Edit a `msgstr` | Kept, but marked unapproved for re-review (`update_option` in `crowdin/crowdin.yml`). | +| Delete a `msgid` | Discarded along with the string. | +| Rename a `msgid` | **Lost.** Crowdin matches by key, so this is a delete plus an untranslated add. | + +**The `.po` files themselves are fixed: never delete, rename or move one.** +A catalog tracked on `main` has to still be there, whether it holds 600 +strings or none — deleting it, renaming it, moving it out of `strings/en/`, +or emptying it in place all fail the build. Delete the individual `msgid` +entries and keep the file. + +The reason is that `crowdin upload sources` can only ever *upload* a file. +A deleted catalog survives in Crowdin and comes back, with all its strings, +on the next pull. A renamed one is worse: the new name uploads as a new +file while the old one stays, so the pull restores **both** and every string +then exists twice. Retiring or renaming a catalog has to start in the +Crowdin UI. The non-English translations are likewise Crowdin's alone. diff --git a/crowdin/crowdin.yml b/crowdin/crowdin.yml index 7cc8d4e..5e6226b 100644 --- a/crowdin/crowdin.yml +++ b/crowdin/crowdin.yml @@ -11,6 +11,12 @@ # Pushes from here must land on these existing Crowdin files, or they'd # create duplicate strings instead of updating existing ones. # +# `update_option` covers the case where a push changes the English text of an +# existing string (these files carry `X-Crowdin-SourceKey: msgstr`, so editing +# a msgstr *is* a source-text change). Crowdin's default is to treat the +# changed string as new and discard its existing translations; +# `update_as_unapproved` keeps them and flags them for re-review instead. +# "project_id": "257" "base_path": ".." "base_url": "https://youversion.crowdin.com" @@ -21,6 +27,7 @@ "source": "strings/en/*.po", "dest": "Bible Loop (Master)/%original_file_name%", "translation": "Bible Loop (Master)/%two_letters_code%/%original_file_name%", + "update_option": "update_as_unapproved", }, ] diff --git a/scripts/crowdin_sync.py b/scripts/crowdin_sync.py index f619f9c..6cd9c03 100755 --- a/scripts/crowdin_sync.py +++ b/scripts/crowdin_sync.py @@ -15,22 +15,22 @@ parsing details. --push - Pushes any new source strings added locally (a contributor hand-edits - the target `.po` file directly, no script needed) to Crowdin, via an - unconditional `crowdin upload sources`, then exits -- no pull, no - commit, no PR. Crowdin matches by identifier, so this is a no-op for - anything unchanged -- no local diffing needed. Does not require - `GH_TOKEN`. + Pushes locally-made source string changes -- additions, edits and + deletions alike (a contributor hand-edits the target `.po` file + directly, no script needed) -- to Crowdin, via an unconditional + `crowdin upload sources`, then exits: no pull, no commit, no PR. + Crowdin matches by identifier, so this is a no-op for anything + unchanged -- no local diffing needed. Does not require `GH_TOKEN`. --dry-run Preview the pull without switching branches, committing, pushing, or opening a PR. Combine with `--push` to preview just the push instead (`crowdin upload sources --dryrun`). Does not require `GH_TOKEN`. -Crowdin is the single source of truth for existing strings; they're never -hand-edited here. New strings are added by hand-editing the target `.po` -file directly and are gated on PRs by `scripts/crowdin_validator.py`, but -only this script ever writes to Crowdin. +Source strings are added, edited and deleted by hand-editing the target +`.po` file directly; `scripts/crowdin_validator.py` reports those deltas on +the PR and blocks only outright mistakes. Crowdin stays the single source of +truth for *translations*, and only this script ever writes to Crowdin. Bundle 13 -- the same bundle youversion-flutter-loop's own pull uses -- was confirmed via a live download to be scoped to exactly "Bible Loop @@ -236,20 +236,64 @@ def changed_po_files(): print("No parent commit (e.g. first commit); uploading all source files.") return None + parent_ref = parent.stdout.strip() diff = capture([ - "git", "diff", "--name-only", "--diff-filter=ACMR", - parent.stdout.strip(), "HEAD", "--", "strings/en/*.po", + "git", "diff", "--name-only", "--no-renames", "--diff-filter=ACM", + parent_ref, "HEAD", "--", "strings/en/*.po", ]) + warn_deleted_files(parent_ref) return [line for line in diff.splitlines() if line] +def warn_deleted_files(parent_ref): + # A deleted .po can't be uploaded, so it falls outside the ACMR filter + # above and would otherwise vanish without a trace: the file stays in + # Crowdin and the next pull restores it, strings and all. + # + # crowdin_validator.py rejects this on the PR, so it should never reach + # here -- but the validator skips crowdin_sync.py's own branch and only + # runs on PRs, so this is the backstop for anything that got in another + # way. Warn loudly rather than failing the push of the files that did + # change; by this point the merge has already happened. + # --no-renames here too: without it a `git mv` of a catalog reports a + # single R entry, which this filter misses entirely -- so the old file + # would stay in Crowdin unmentioned while the new one is uploaded + # alongside it. As a delete plus an add, the removal is visible. + removed = capture([ + "git", "diff", "--name-only", "--no-renames", "--diff-filter=D", + parent_ref, "HEAD", "--", "strings/en/*.po", + ]) + for path in removed.splitlines(): + if path: + name = Path(path).name + print( + f"WARNING: {path} was deleted locally but is NOT removed from " + f"Crowdin by this push. Delete \"{DEST_PATTERN.split('%')[0]}{name}\" " + "in the Crowdin UI, or it will come back -- with every string " + "it held -- on the next pull.", + flush=True, + ) + + def push_new_strings(dry_run=False): - # Push any locally-added source strings (hand-edited into the target + # Push locally-made source string changes (hand-edited into the target # .po file directly; see scripts/crowdin_validator.py for the PR-time - # gate) to Crowdin. This is unconditional per-file: Crowdin matches by + # report) to Crowdin. This is unconditional per-file: Crowdin matches by # identifier and no-ops anything unchanged, so no per-string diffing is # needed here -- just per-file scoping (see changed_po_files() above). - # Never pass --delete-obsolete; this must stay additive/update-only. + # + # Additions, edits AND deletions all propagate from this one command, via + # the CLI's default --auto-update: Crowdin re-reads the uploaded source + # file and reconciles the whole file against it, so a key dropped locally + # goes obsolete in Crowdin too. crowdin.yml's `update_option` decides what + # an *edit* costs (see that file). + # + # Never pass --delete-obsolete. Despite the name it does not delete + # obsolete strings -- per `crowdin upload sources --help` (CLI 4.12.0) it + # deletes "obsolete files and folders ... that no longer match the source + # configuration", and since each invocation below is scoped with + # `-s `, the other 29 .po files in "Bible Loop (Master)/" would + # all look obsolete and be deleted. files = changed_po_files() if files == []: print("No strings/en/*.po changes vs. the previous commit; nothing to push.") diff --git a/scripts/crowdin_validator.py b/scripts/crowdin_validator.py index 721e006..60bac8e 100755 --- a/scripts/crowdin_validator.py +++ b/scripts/crowdin_validator.py @@ -1,16 +1,36 @@ #!/usr/bin/env python3 -"""CI gate: fail a PR if it edits or deletes an existing Crowdin string. - -Counterpart to `scripts/crowdin_sync.py` (which pulls translations and pushes -new source strings). Crowdin remains the single source of truth: existing -strings can only be edited or deleted via the Crowdin UI. New strings are -added by hand-editing the target `.po` file directly under `strings/en/` -- -no script needed for that part. - -This script fails if any `.po` file changed in the current branch, vs. the -base branch, edited or deleted an existing entry, or introduced a "new" key -that collides with an existing key in a different, unchanged file. New, -non-colliding entries are the expected delta. +"""CI gate for hand-edited Crowdin source strings. + +Counterpart to `scripts/crowdin_sync.py` (which pulls source strings and +pushes local ones). Adds, edits and deletes are all made by hand-editing the +target `.po` file directly under `strings/en/` -- no script needed for that +part, and no Crowdin UI round-trip required. + +This script diffs every `.po` file changed in the current branch, vs. the +base branch, and splits what it finds in two: + + NOTE (reported, does not fail the build) + Added, edited, deleted and renamed strings. These are all legitimate + deltas; the notices exist so a reviewer reading the CI log can see + exactly which existing strings a PR touched, and what it costs on the + Crowdin side (an edit unapproves that string's translations; a rename + loses them outright). + + ERROR (fails the build) + A key defined twice in one file, an entry this script cannot parse, or + an entry with an empty value -- each of these is either ambiguous or + invisible to the diff above, so it has to stop the build. Plus a new + key that duplicates one in a different file: Crowdin scopes keys per + file and tolerates that (53 such pairs already exist on main), but it + is almost always a copy-paste slip, so it is worth a deliberate second + look rather than a silent merge. + + Losing a whole file is an error too, reported as a rename/move or as + a removal -- see missing_file() below. Deleting strings is fine; + deleting or renaming the file that holds them is not. + +Structural checks run only over the files the PR actually changed, so a +pre-existing quirk elsewhere can never fail an unrelated PR. Pure git + local `.po` parsing -- never touches the Crowdin API or needs `CROWDIN_API_TOKEN`. Intended to run in Bitrise on PRs touching @@ -25,6 +45,7 @@ import re import subprocess import sys +from collections import Counter from pathlib import Path EN_STRINGS_DIR = Path("strings/en") @@ -32,11 +53,8 @@ # Must match crowdin_sync.py's BRANCH default/env var exactly: PRs from this # rolling branch are crowdin_sync.py's own pull-mode output, mirroring -# whatever Crowdin's UI/API currently has -- including edits/deletions, which -# is exactly what this validator otherwise exists to block for *hand*-edited -# PRs. Skip entirely for that branch, or crowdin_pull's own self-heal PRs -# (e.g. reflecting a string deleted on Crowdin) would fail this check and -# never be mergeable. +# whatever Crowdin's UI/API currently has. It is generated, never reviewed as +# a hand-edit, so none of the checks below tell us anything useful about it. SYNC_BRANCH = os.environ.get("CROWDIN_SYNC_BRANCH", "chore/crowdin-sync") # Matches any `msgid "..."` line (including ones followed by msgid_plural) -- @@ -54,6 +72,15 @@ re.MULTILINE, ) +# Pulls the value out of each msgstr line of an already-matched entry body. +_MSGSTR_VALUE_RE = re.compile(r'^msgstr(?:\[\d+\])? "((?:[^"\\]|\\.)*)"$', re.MULTILINE) + +# A bare quoted line -- PO's multi-line continuation syntax. _ENTRY_RE stops +# at the first msgstr line, so an entry continued this way parses as an entry +# with an empty value and the continuation is silently dropped; matched right +# after an entry body, this is how we catch that. +_CONTINUATION_RE = re.compile(r'"(?:[^"\\]|\\.)*"[ \t]*(?:\r?\n|$)') + _ESCAPE_RE = re.compile(r"\\(.)") _UNESCAPE_MAP = {"n": "\n", "t": "\t", '"': '"', "\\": "\\"} @@ -71,13 +98,17 @@ def find_existing_keys(strings_dir: Path) -> dict[str, set[str]]: keys: dict[str, set[str]] = {} for po_path in sorted(strings_dir.glob("*.po")): content = po_path.read_text(encoding="utf-8") - for match in _ANY_MSGID_RE.finditer(content): - key = po_unescape(match.group(1)) - if key: - keys.setdefault(key, set()).add(po_path.name) + for key in all_keys(content): + keys.setdefault(key, set()).add(po_path.name) return keys +def all_keys(content: str) -> list[str]: + """Every non-header msgid in the file, in order, including duplicates.""" + keys = [po_unescape(match.group(1)) for match in _ANY_MSGID_RE.finditer(content)] + return [key for key in keys if key] + + def parse_entries(content: str) -> dict[str, str]: return { po_unescape(match.group(1)): match.group(2) @@ -86,9 +117,157 @@ def parse_entries(content: str) -> dict[str, str]: } +def duplicate_keys(content: str) -> list[tuple[str, int]]: + """Keys defined more than once in a single file. + + parse_entries() silently keeps only the last definition, so without this + check a duplicate would make one of the two entries invisible to the + add/edit/delete diff below -- and ambiguous on the Crowdin side. + """ + counts = Counter(all_keys(content)) + return sorted((key, count) for key, count in counts.items() if count > 1) + + +def malformed_keys(content: str) -> list[str]: + """Keys this script cannot faithfully round-trip. + + Catches hand-edits that put a `#.`/`#:` comment between msgid and msgstr + or dropped the msgstr entirely (invisible to _ENTRY_RE), and ones that + used PO multi-line continuation (parsed, but with the continuation lines + silently discarded). Either way the entry's real value would be invisible + to the diff, so it has to fail rather than pass -- the canonical + single-line shape is what update_strings.py always writes. + """ + parsed = parse_entries(content) + bad = {key for key in all_keys(content) if key not in parsed} + for match in _ENTRY_RE.finditer(content): + key = po_unescape(match.group(1)) + if key and _CONTINUATION_RE.match(content, match.end()): + bad.add(key) + return sorted(bad) + + +def empty_value_keys(entries: dict[str, str]) -> list[str]: + """Keys whose every msgstr value is empty -- a key with no English text.""" + return sorted( + key + for key, body in entries.items() + if not any(_MSGSTR_VALUE_RE.findall(body)) + ) + + +def renamed_po_files() -> dict[Path, Path]: + """{old path: new path} for every `.po` git sees as renamed or moved. + + Reporting only. changed_po_files() deliberately passes --no-renames, so a + rename decomposes into the delete plus the add it really is on Crowdin's + side; this second pass turns detection back on purely to say *where* a + vanished catalog went. Deliberately unscoped: a move out of strings/en/ + shows up as a plain delete in the path-scoped diff, because the + destination no longer matches the pathspec. + """ + output = capture( + ["git", "diff", "--name-status", "--find-renames", f"origin/{BASE_BRANCH}...HEAD"] + ) + renames = {} + for line in output.splitlines(): + parts = line.split("\t") + if len(parts) == 3 and parts[0].startswith("R"): + source, destination = Path(parts[1]), Path(parts[2]) + if source.suffix == ".po" and EN_STRINGS_DIR in source.parents: + renames[source] = destination + return renames + + +def exists_in_base(path: Path) -> bool: + """Whether the base branch tracks this path at all. + + Distinct from `base_content(path)` being empty, which is also what an + existing-but-empty file looks like -- licenses.po ships with zero + entries, so "held no strings" must not be mistaken for "wasn't there". + """ + return subprocess.run( + ["git", "cat-file", "-e", f"origin/{BASE_BRANCH}:{path.as_posix()}"], + capture_output=True, + ).returncode == 0 + + +def missing_file( + path: Path, + base: dict[str, str], + current: dict[str, str], + renames: dict[Path, Path], +) -> tuple[str, str] | None: + """Classify a `.po` this PR loses as ("moved"|"removed", detail). + + Deleting individual strings is supported; losing the file holding them is + not. `crowdin upload sources` can only ever *upload* a file, so a + vanished file is invisible to the push: it stays in the Crowdin project + and the next pull restores it, along with every string in it. + + The rule is about the file, not its contents -- a catalog tracked on the + base branch has to still be there, whether it held 600 strings or none. + Counting strings instead would wave through deleting or renaming + licenses.po, which is empty but is still a real tracked file. + + The three routes differ enough to be worth naming, since the fix for each + differs. A rename or move uploads the new path and strands the old one, + so the pull brings back *both* and every string ends up duplicated across + two files. A delete simply comes back. Emptying a file in place is the + delete case by another route, and is the one variant that does turn on + contents: it only counts as a loss if there were entries to lose. + """ + if not exists_in_base(path): + return None + destination = renames.get(path) + if destination: + return ("moved", f"{path} -> {destination} ({len(base)} string(s))") + if not path.exists(): + return ("removed", f"{path} (deleted, {len(base)} string(s))") + if base and not current: + return ("removed", f"{path} (emptied, {len(base)} string(s))") + return None + + +def entry_text(body: str) -> str: + """Human-readable rendering of an entry body, for the CI log.""" + return " / ".join(po_unescape(value) for value in _MSGSTR_VALUE_RE.findall(body)) + + +def detect_renames( + deleted: dict[str, str], added: dict[str, str] +) -> list[tuple[str, str, str]]: + """Pair deleted keys with added keys carrying identical text. + + A rename is the one delta where existing translations do not survive: + Crowdin matches strings by identifier, so the new key arrives untranslated + while the old key's translations are discarded with it. Worth calling out + separately instead of burying it in the delete list. + """ + unclaimed = dict(added) + renames = [] + for old_key, body in sorted(deleted.items()): + for new_key, new_body in sorted(unclaimed.items()): + if new_body == body: + renames.append((old_key, new_key, body)) + del unclaimed[new_key] + break + return renames + + def changed_po_files() -> list[Path]: + # --no-renames matters. With git's default rename detection a `git mv` of + # a whole catalog reports only the destination path, so the vanished + # source file never reaches removed_file() and the rename sails through as + # a pile of "added" strings -- while the push uploads the new file and + # leaves the old one in Crowdin, so the next pull restores both and every + # string exists twice. Treating a rename as a delete plus an add surfaces + # the removal, which is what it actually is as far as Crowdin's concerned. + # It also catches a move *out* of strings/en/, which rename detection + # would otherwise hide from this path-scoped diff. output = capture( - ["git", "diff", "--name-only", f"origin/{BASE_BRANCH}...HEAD", "--", str(EN_STRINGS_DIR)] + ["git", "diff", "--name-only", "--no-renames", + f"origin/{BASE_BRANCH}...HEAD", "--", str(EN_STRINGS_DIR)] ) return [Path(line) for line in output.splitlines() if line.endswith(".po")] @@ -102,6 +281,14 @@ def base_content(path: Path) -> str: return result.stdout if result.returncode == 0 else "" +def report(label: str, items: list[str]) -> None: + if not items: + return + print(label) + for item in items: + print(f" - {item}") + + def validate() -> int: changed = changed_po_files() if not changed: @@ -109,17 +296,32 @@ def validate() -> int: return 0 existing = find_existing_keys(EN_STRINGS_DIR) - edited, deleted, collisions = [], [], [] + file_renames = renamed_po_files() + added, edited, deleted, renamed = [], [], [], [] + collisions, duplicates, malformed, empties = [], [], [], [] + moved, removed = [], [] for path in changed: + content = path.read_text(encoding="utf-8") if path.exists() else "" base = parse_entries(base_content(path)) - current = parse_entries(path.read_text(encoding="utf-8")) if path.exists() else {} - - for key, base_text in base.items(): - if key not in current: - deleted.append(f"{key} ({path})") - elif current[key] != base_text: - edited.append(f"{key} ({path})") + current = parse_entries(content) + + wiped = missing_file(path, base, current, file_renames) + if wiped: + # Report the file once rather than every string it held. + kind, detail = wiped + (moved if kind == "moved" else removed).append(detail) + continue + + unparseable = malformed_keys(content) + for key, count in duplicate_keys(content): + duplicates.append(f"{key} ({path}, defined {count}x)") + for key in unparseable: + malformed.append(f"{key} ({path})") + # A continued entry also looks empty; report it once, as unparseable. + for key in empty_value_keys(current): + if key not in unparseable: + empties.append(f"{key} ({path})") for key in current: if key in base: @@ -128,17 +330,77 @@ def validate() -> int: if other_files: collisions.append(f"{key} ({path}, also in {', '.join(sorted(other_files))})") - if not edited and not deleted and not collisions: - print("OK: only new, non-colliding strings added.") + gone = {key: body for key, body in base.items() if key not in current} + new = {key: body for key, body in current.items() if key not in base} + renames = detect_renames(gone, new) + + for old_key, new_key, body in renames: + renamed.append(f'{old_key} -> {new_key} ({path}): "{entry_text(body)}"') + del gone[old_key] + del new[new_key] + + for key in sorted(new): + added.append(f'{key} ({path}): "{entry_text(new[key])}"') + for key in sorted(gone): + deleted.append(f'{key} ({path}): "{entry_text(gone[key])}"') + for key in sorted(base): + if key in current and current[key] != base[key]: + edited.append( + f'{key} ({path}): "{entry_text(base[key])}" -> "{entry_text(current[key])}"' + ) + + summary = ( + f"{len(changed)} file(s) changed: {len(added)} added, {len(edited)} edited, " + f"{len(deleted)} deleted, {len(renamed)} renamed." + ) + if moved: + summary += f" {len(moved)} file(s) renamed/moved." + if removed: + summary += f" {len(removed)} file(s) removed." + print(summary) + + report("NOTE: string(s) added:", added) + report( + "NOTE: existing string(s) edited -- Crowdin keeps the existing " + "translations but marks them unapproved for re-review:", + edited, + ) + report( + "NOTE: existing string(s) deleted -- removed from Crowdin by the next " + "`crowdin_sync.py --push`:", + deleted, + ) + report( + "NOTE: likely rename(s) -- Crowdin matches by key, so the existing " + "translations do NOT carry over to the new key:", + renamed, + ) + + if not (moved or removed or collisions or duplicates or malformed or empties): return 0 - if edited: - print(f"ERROR: existing string(s) edited outside the Crowdin UI: {', '.join(edited)}") - if deleted: - print(f"ERROR: existing string(s) deleted outside the Crowdin UI: {', '.join(deleted)}") - if collisions: - print(f"ERROR: new string(s) collide with an existing key elsewhere: {', '.join(collisions)}") - print("Existing strings can only be changed via the Crowdin UI.") + report( + "ERROR: whole file(s) renamed or moved -- there is no rename on the " + "push path: the new name is uploaded as a new file and the old one " + "stays, so the next pull restores BOTH and every string exists twice. " + "Rename the file in the Crowdin UI instead, then pull:", + moved, + ) + report( + "ERROR: whole file(s) removed -- delete the individual strings instead " + "and keep the file, or remove the file in the Crowdin UI first " + "(`crowdin upload sources` can only upload, so a removed file survives " + "in Crowdin and comes back on the next pull):", + removed, + ) + report("ERROR: new string(s) duplicate an existing key in another file:", collisions) + report("ERROR: key(s) defined more than once in the same file:", duplicates) + report( + "ERROR: unparseable entry -- msgstr must be a single escaped line " + "immediately after msgid (see update_strings.py's render_entries):", + malformed, + ) + report("ERROR: entry with an empty value:", empties) return 1 diff --git a/scripts/test_crowdin_validator.py b/scripts/test_crowdin_validator.py new file mode 100644 index 0000000..b95edd0 --- /dev/null +++ b/scripts/test_crowdin_validator.py @@ -0,0 +1,445 @@ +#!/usr/bin/env python3 +"""Tests for scripts/crowdin_validator.py. + +Stdlib `unittest` only -- this repo has no test dependencies and no package +manifest, so the suite has to run on a bare `python3`: + + python3 -m unittest discover -s scripts -p 'test_*.py' -v + +Two layers. `ParsingTests` exercise the pure `.po` parsing/detection helpers +directly. `ValidatorEndToEndTests` build a throwaway git repo (bare origin + +working clone) per case and run the script as a subprocess, because the exit +code and stdout -- not any Python API -- are what Bitrise actually consumes. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +import crowdin_validator as validator # noqa: E402 + +VALIDATOR = Path(__file__).resolve().parent / "crowdin_validator.py" + +# Identity for the throwaway repos, passed per-command in the environment +# rather than written with `git config`. A stray `git config` runs against +# whatever repo the process happens to be in, so a bad cwd would silently +# rewrite the real repo's .git/config -- and misattribute its next commit. +# Env vars cannot leak that way. +GIT_ENV = { + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@example.com", +} + +HEADER = 'msgid ""\nmsgstr ""\n"Language: en-US\\n"\n' + + +def po(*blocks: str) -> str: + """Assemble a .po file the way update_strings.py's write_po_file does.""" + return f"{HEADER}\n" + "\n\n".join(blocks) + "\n\n" + + +def entry(key: str, value: str) -> str: + return f'msgid "{key}"\nmsgstr "{value}"' + + +def plural(key: str, *values: str) -> str: + forms = "\n".join(f'msgstr[{i}] "{v}"' for i, v in enumerate(values)) + return f'msgid "{key}"\nmsgid_plural "{key}"\n{forms}' + + +class ParsingTests(unittest.TestCase): + def test_parses_single_line_entries_and_skips_the_header(self): + entries = validator.parse_entries(po(entry("about", "About"), entry("bible", "Bible"))) + self.assertEqual(sorted(entries), ["about", "bible"]) + self.assertEqual(validator.entry_text(entries["about"]), "About") + + def test_parses_plural_entries(self): + entries = validator.parse_entries(po(plural("xHours", "1 hour", "%1$s hours"))) + self.assertEqual(list(entries), ["xHours"]) + self.assertEqual(validator.entry_text(entries["xHours"]), "1 hour / %1$s hours") + + def test_entry_text_unescapes(self): + entries = validator.parse_entries(po(entry("greeting", 'Hi \\"you\\"\\nthere'))) + self.assertEqual(validator.entry_text(entries["greeting"]), 'Hi "you"\nthere') + + def test_editing_one_plural_form_changes_the_body(self): + before = validator.parse_entries(po(plural("xHours", "1 hour", "%1$s hours"))) + after = validator.parse_entries(po(plural("xHours", "1 hour", "%1$s hrs"))) + self.assertNotEqual(before["xHours"], after["xHours"]) + + def test_duplicate_keys_are_counted(self): + content = po(entry("about", "About"), entry("about", "About Us"), entry("bible", "Bible")) + self.assertEqual(validator.duplicate_keys(content), [("about", 2)]) + + def test_no_duplicates_reported_for_a_clean_file(self): + self.assertEqual(validator.duplicate_keys(po(entry("about", "About"))), []) + + def test_malformed_detects_multi_line_continuation(self): + content = po('msgid "blurb"\nmsgstr ""\n"a long line "\n"continued"') + self.assertEqual(validator.malformed_keys(content), ["blurb"]) + + def test_malformed_detects_a_comment_between_msgid_and_msgstr(self): + content = po('msgid "about"\n#. translator note\nmsgstr "About"') + self.assertEqual(validator.malformed_keys(content), ["about"]) + + def test_malformed_detects_a_missing_msgstr(self): + content = po('msgid "about"', entry("bible", "Bible")) + self.assertEqual(validator.malformed_keys(content), ["about"]) + + def test_well_formed_entries_are_not_malformed(self): + content = po(entry("about", "About"), plural("xHours", "1 hour", "%1$s hours")) + self.assertEqual(validator.malformed_keys(content), []) + + def test_empty_value_keys(self): + entries = validator.parse_entries(po(entry("about", ""), entry("bible", "Bible"))) + self.assertEqual(validator.empty_value_keys(entries), ["about"]) + + def test_detect_renames_pairs_matching_bodies(self): + gone = validator.parse_entries(po(entry("badges", "Badges"))) + new = validator.parse_entries(po(entry("userBadges", "Badges"))) + self.assertEqual(validator.detect_renames(gone, new), [("badges", "userBadges", gone["badges"])]) + + def test_detect_renames_ignores_unrelated_add_and_delete(self): + gone = validator.parse_entries(po(entry("badges", "Badges"))) + new = validator.parse_entries(po(entry("streaks", "Streaks"))) + self.assertEqual(validator.detect_renames(gone, new), []) + + def test_detect_renames_ignores_a_merge(self): + # Folding one string's text into another rewrites the surviving body, + # so there is no identical pair to match -- a merge is a delete plus + # an edit, never a rename. + gone = validator.parse_entries(po(entry("welcomeBody", "Glad you're here"))) + new = validator.parse_entries(po(entry("welcomeTitle", "Welcome -- glad you're here"))) + self.assertEqual(validator.detect_renames(gone, new), []) + + def test_detect_renames_pairs_a_merge_that_keeps_one_body_verbatim(self): + # Known limit of the heuristic: if a merge happens to leave one of the + # deleted bodies byte-identical, that half is indistinguishable from a + # rename and is reported as one. The notice is advisory, and the + # translation consequence it warns about is the same either way. + gone = validator.parse_entries(po(entry("partA", "Hello"), entry("partB", "World"))) + new = validator.parse_entries(po(entry("merged", "Hello"))) + renames = validator.detect_renames(gone, new) + self.assertEqual([(old, key) for old, key, _ in renames], [("partA", "merged")]) + + def test_detect_renames_claims_each_added_key_once(self): + gone = validator.parse_entries(po(entry("a", "Same"), entry("b", "Same"))) + new = validator.parse_entries(po(entry("c", "Same"))) + renames = validator.detect_renames(gone, new) + self.assertEqual([(old, new_key) for old, new_key, _ in renames], [("a", "c")]) + + +class ValidatorEndToEndTests(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + root = Path(self._tmp.name) + self.origin = root / "origin.git" + self.repo = root / "work" + + self.git("init", "--bare", "--initial-branch=main", str(self.origin), cwd=root) + self.git("init", "--initial-branch=main", str(self.repo), cwd=root) + self.git("remote", "add", "origin", str(self.origin)) + (self.repo / "strings" / "en").mkdir(parents=True) + + def git(self, *args: str, cwd: Path | None = None) -> None: + subprocess.run( + ["git", "-c", "commit.gpgsign=false", *args], + cwd=str(cwd or self.repo), + env={**os.environ, **GIT_ENV}, + check=True, + capture_output=True, + text=True, + ) + + def write(self, name: str, content: str) -> None: + (self.repo / "strings" / "en" / name).write_text(content, encoding="utf-8") + + def commit(self, message: str) -> None: + self.git("add", "-A") + self.git("commit", "-m", message) + + def commit_base(self, **files: str) -> None: + for name, content in files.items(): + self.write(f"{name}.po", content) + self.commit("base") + self.git("push", "-u", "origin", "main") + self.git("switch", "-c", "feature") + + def run_validator(self, branch: str | None = None) -> subprocess.CompletedProcess: + env = {k: v for k, v in os.environ.items() if k not in ("BITRISE_GIT_BRANCH", "GITHUB_HEAD_REF")} + env["CROWDIN_SYNC_BASE"] = "main" + if branch is not None: + env["BITRISE_GIT_BRANCH"] = branch + return subprocess.run( + [sys.executable, str(VALIDATOR)], + cwd=str(self.repo), + env=env, + capture_output=True, + text=True, + ) + + def assertPasses(self, result: subprocess.CompletedProcess) -> str: + self.assertEqual(result.returncode, 0, msg=f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}") + return result.stdout + + def assertFails(self, result: subprocess.CompletedProcess) -> str: + self.assertEqual(result.returncode, 1, msg=f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}") + return result.stdout + + # -- notices (exit 0) ------------------------------------------------- + + def test_no_po_changes_passes(self): + self.commit_base(app=po(entry("about", "About"))) + (self.repo / "README.md").write_text("hello\n", encoding="utf-8") + self.commit("unrelated") + self.assertIn("no strings changed", self.assertPasses(self.run_validator())) + + def test_addition_passes(self): + self.commit_base(app=po(entry("about", "About"))) + self.write("app.po", po(entry("about", "About"), entry("bible", "Bible"))) + self.commit("add") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("1 added, 0 edited, 0 deleted, 0 renamed", stdout) + self.assertIn("bible", stdout) + + def test_edit_is_reported_but_passes(self): + self.commit_base(app=po(entry("about", "About"))) + self.write("app.po", po(entry("about", "About Us"))) + self.commit("edit") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("0 added, 1 edited, 0 deleted, 0 renamed", stdout) + self.assertIn("unapproved", stdout) + self.assertIn('"About" -> "About Us"', stdout) + + def test_delete_is_reported_but_passes(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.write("app.po", po(entry("about", "About"))) + self.commit("delete") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("0 added, 0 edited, 1 deleted, 0 renamed", stdout) + self.assertIn("deleted", stdout) + self.assertIn("bible", stdout) + + def test_rename_is_reported_separately(self): + self.commit_base(app=po(entry("badges", "Badges"))) + self.write("app.po", po(entry("userBadges", "Badges"))) + self.commit("rename") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("0 added, 0 edited, 0 deleted, 1 renamed", stdout) + self.assertIn("badges -> userBadges", stdout) + self.assertIn("do NOT carry over", stdout) + + def test_merging_one_string_into_another_passes(self): + self.commit_base( + app=po(entry("welcomeTitle", "Welcome"), entry("welcomeBody", "Glad you're here")) + ) + self.write("app.po", po(entry("welcomeTitle", "Welcome -- glad you're here"))) + self.commit("merge body into title") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("0 added, 1 edited, 1 deleted, 0 renamed", stdout) + self.assertIn( + "welcomeTitle (strings/en/app.po): \"Welcome\" -> \"Welcome -- glad you\'re here\"", + stdout, + ) + self.assertIn("welcomeBody (strings/en/app.po): \"Glad you\'re here\"", stdout) + self.assertNotIn("likely rename(s)", stdout) + + def test_merging_two_strings_into_a_new_key_is_not_a_rename(self): + self.commit_base(app=po(entry("partA", "Hello"), entry("partB", "World"))) + self.write("app.po", po(entry("greeting", "Hello World"))) + self.commit("merge two into a new key") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("1 added, 0 edited, 2 deleted, 0 renamed", stdout) + self.assertNotIn("likely rename(s)", stdout) + + def test_merging_a_string_into_another_file_passes(self): + self.commit_base( + app=po(entry("theme", "Theme")), + settings=po(entry("appearance", "Appearance")), + ) + self.write("app.po", HEADER + "\n" + entry("unrelated", "Unrelated") + "\n\n") + self.write("settings.po", po(entry("appearance", "Appearance & theme"))) + self.commit("move theme text into settings") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("1 added, 1 edited, 1 deleted, 0 renamed", stdout) + self.assertIn("theme (strings/en/app.po)", stdout) + self.assertIn("appearance (strings/en/settings.po)", stdout) + + def test_edited_plural_is_reported(self): + self.commit_base(search=po(plural("xHours", "1 hour", "%1$s hours"))) + self.write("search.po", po(plural("xHours", "1 hour", "%1$s hrs"))) + self.commit("edit plural") + self.assertIn("1 edited", self.assertPasses(self.run_validator())) + + def test_reordering_entries_is_not_a_change(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.write("app.po", po(entry("bible", "Bible"), entry("about", "About"))) + self.commit("reorder") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("0 added, 0 edited, 0 deleted, 0 renamed", stdout) + + def test_sync_branch_is_skipped_entirely(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.write("app.po", po(entry("about", "About"))) + self.commit("bot delete") + stdout = self.assertPasses(self.run_validator(branch="chore/crowdin-sync")) + self.assertIn("skipping validation", stdout) + + # -- errors (exit 1) -------------------------------------------------- + + def test_cross_file_collision_fails(self): + self.commit_base(app=po(entry("about", "About")), settings=po(entry("theme", "Theme"))) + self.write("settings.po", po(entry("theme", "Theme"), entry("about", "About"))) + self.commit("collide") + stdout = self.assertFails(self.run_validator()) + self.assertIn("duplicate an existing key in another file", stdout) + self.assertIn("app.po", stdout) + + def test_duplicate_key_in_one_file_fails(self): + self.commit_base(app=po(entry("about", "About"))) + self.write("app.po", po(entry("about", "About"), entry("bible", "Bible"), entry("bible", "Bible!"))) + self.commit("duplicate") + stdout = self.assertFails(self.run_validator()) + self.assertIn("defined more than once", stdout) + self.assertIn("bible", stdout) + + def test_malformed_entry_fails(self): + self.commit_base(app=po(entry("about", "About"))) + self.write("app.po", po(entry("about", "About"), 'msgid "blurb"\nmsgstr ""\n"split "\n"line"')) + self.commit("malformed") + self.assertIn("unparseable entry", self.assertFails(self.run_validator())) + + def test_deleting_a_whole_file_fails(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + (self.repo / "strings" / "en" / "app.po").unlink() + self.commit("drop file") + stdout = self.assertFails(self.run_validator()) + self.assertIn("whole file(s) removed", stdout) + self.assertIn("deleted, 2 string(s)", stdout) + self.assertIn("1 file(s) removed", stdout) + # A plain delete must not be dressed up as a rename. + self.assertNotIn("renamed or moved", stdout) + + def test_renaming_a_whole_file_fails(self): + # git's rename detection reports only the destination path, which + # would hide the removal and let the rename through as a pile of + # additions -- while Crowdin kept the original file and the next pull + # restored both catalogs. changed_po_files() passes --no-renames. + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.git("mv", "strings/en/app.po", "strings/en/application.po") + self.commit("rename catalog") + stdout = self.assertFails(self.run_validator()) + self.assertIn("whole file(s) renamed or moved", stdout) + self.assertIn( + "strings/en/app.po -> strings/en/application.po (2 string(s))", stdout + ) + self.assertIn("1 file(s) renamed/moved", stdout) + # Diagnosed as a rename, not misreported as a plain delete. + self.assertNotIn("whole file(s) removed", stdout) + + def test_moving_a_file_out_of_the_strings_dir_fails(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + (self.repo / "strings" / "archive").mkdir(parents=True) + self.git("mv", "strings/en/app.po", "strings/archive/app.po") + self.commit("archive catalog") + stdout = self.assertFails(self.run_validator()) + # The path-scoped diff sees only a delete here; renamed_po_files() + # runs unscoped precisely so the destination is still named. + self.assertIn("whole file(s) renamed or moved", stdout) + self.assertIn( + "strings/en/app.po -> strings/archive/app.po (2 string(s))", stdout + ) + + def test_renaming_one_file_while_deleting_another_reports_each_kind(self): + self.commit_base( + app=po(entry("about", "About")), + settings=po(entry("theme", "Theme"), entry("mode", "Mode")), + ) + self.git("mv", "strings/en/app.po", "strings/en/application.po") + (self.repo / "strings" / "en" / "settings.po").unlink() + self.commit("rename one, delete another") + stdout = self.assertFails(self.run_validator()) + self.assertIn("1 file(s) renamed/moved", stdout) + self.assertIn("1 file(s) removed", stdout) + self.assertIn("strings/en/app.po -> strings/en/application.po (1 string(s))", stdout) + self.assertIn("strings/en/settings.po (deleted, 2 string(s))", stdout) + + def test_deleting_an_empty_catalog_fails(self): + # licenses.po ships with zero entries but is still a tracked file. + # The rule is about losing the file, not about losing strings. + self.commit_base(app=po(entry("about", "About")), licenses=HEADER) + (self.repo / "strings" / "en" / "licenses.po").unlink() + self.commit("delete the empty catalog") + stdout = self.assertFails(self.run_validator()) + self.assertIn("whole file(s) removed", stdout) + self.assertIn("strings/en/licenses.po (deleted, 0 string(s))", stdout) + + def test_renaming_an_empty_catalog_fails(self): + self.commit_base(app=po(entry("about", "About")), licenses=HEADER) + self.git("mv", "strings/en/licenses.po", "strings/en/attributions.po") + self.commit("rename the empty catalog") + stdout = self.assertFails(self.run_validator()) + self.assertIn("whole file(s) renamed or moved", stdout) + self.assertIn( + "strings/en/licenses.po -> strings/en/attributions.po (0 string(s))", stdout + ) + + def test_adding_a_brand_new_catalog_passes(self): + # A file absent from the base branch cannot be a loss. + self.commit_base(app=po(entry("about", "About"))) + self.write("movies.po", po(entry("trailer", "Trailer"))) + self.commit("add a new catalog") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("1 added", stdout) + self.assertNotIn("ERROR", stdout) + + def test_emptying_a_file_in_place_fails(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.write("app.po", HEADER) + self.commit("empty file") + stdout = self.assertFails(self.run_validator()) + self.assertIn("whole file(s) removed", stdout) + self.assertIn("emptied, 2 string(s)", stdout) + + def test_deleting_all_but_one_string_still_passes(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.write("app.po", po(entry("about", "About"))) + self.commit("keep one") + self.assertIn("1 deleted", self.assertPasses(self.run_validator())) + + def test_touching_an_already_empty_file_is_not_flagged(self): + # licenses.po ships empty; adding to it must not look like a removal. + self.commit_base(app=po(entry("about", "About")), licenses=HEADER) + self.write("licenses.po", po(entry("mitLicense", "MIT License"))) + self.commit("populate empty file") + stdout = self.assertPasses(self.run_validator()) + self.assertIn("1 added", stdout) + self.assertNotIn("removed", stdout) + + def test_empty_value_fails(self): + self.commit_base(app=po(entry("about", "About"))) + self.write("app.po", po(entry("about", "About"), entry("bible", ""))) + self.commit("empty") + self.assertIn("empty value", self.assertFails(self.run_validator())) + + def test_notices_are_still_printed_alongside_an_error(self): + self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) + self.write("app.po", po(entry("about", "About Us"), entry("bible", "Bible"), entry("bible", "Bible"))) + self.commit("mixed") + stdout = self.assertFails(self.run_validator()) + self.assertIn("1 edited", stdout) + self.assertIn("defined more than once", stdout) + + +if __name__ == "__main__": + unittest.main()