From 5542fab0f25379de8c0e75d5f52f377e49228880 Mon Sep 17 00:00:00 2001 From: Anthony Raj Date: Mon, 24 Aug 2026 14:31:26 -0500 Subject: [PATCH 1/4] feat(crowdin): allow source string edits and deletes The validator was an additions-only gate: any PR that edited or deleted an existing string failed, on the policy that Crowdin was the sole origin for those changes. Make the repo a legitimate origin for all three. crowdin_validator.py now splits its findings in two. Edits, deletes and renames become NOTEs -- reported with old -> new text so a reviewer can see what a PR touched, but no longer failing the build. What still fails is a mistake: a key defined twice in one file, an entry the parser cannot round-trip, or an entry with an empty value. Cross-file duplicate keys keep failing too, though the message no longer claims Crowdin rejects them (53 such pairs already exist on main). Structural checks are scoped to the files a PR changed, so a pre-existing quirk elsewhere cannot fail an unrelated PR. Deleting strings is allowed; deleting the file that holds them is not, and that stays an error. `crowdin upload sources` can only ever upload a file, so a removed .po survives in Crowdin and the next pull restores it along with every string in it -- retiring a whole file has to start in the Crowdin UI. Emptying a file in place is the same outcome by another route and is caught the same way, though only for a file that had entries to begin with, since licenses.po ships empty. Such a file is reported once rather than as a flood of individual delete notices. Renames are detected by pairing a deleted key with an added key carrying identical text, and called out separately: that is the one delta where Crowdin's key matching means the translations do not survive. Relaxing the validator alone would have been cosmetic, so two coupled fixes come with it. crowdin.yml gains update_option: update_as_unapproved -- these files carry X-Crowdin-SourceKey: msgstr, so editing a msgstr is a source-text change, and Crowdin's default would have discarded that string's translations in every language. crowdin_sync.py warns when a whole .po is deleted: the validator rejects that on the PR, but it skips the sync bot's own branch and only runs on PRs, so the push keeps a backstop for anything that got in another way. The "never pass --delete-obsolete" comment was right but for the wrong reason, and is corrected: per crowdin upload sources --help (CLI 4.12.0) the flag deletes obsolete *files and folders*, not strings, so against our per-file `-s` invocation it would have deleted the other 29 .po files. Per-string deletion already propagates via the default --auto-update. Adds scripts/test_crowdin_validator.py -- the repo's first tests. Stdlib unittest, no new dependencies: parsing/detection unit tests plus end-to-end cases that build a throwaway git repo and assert on exit code and stdout, which is the whole contract Bitrise consumes. python3 -m unittest discover -s scripts -p 'test_*.py' --- README.md | 48 ++-- crowdin/crowdin.yml | 7 + scripts/crowdin_sync.py | 68 ++++-- scripts/crowdin_validator.py | 259 ++++++++++++++++++--- scripts/test_crowdin_validator.py | 370 ++++++++++++++++++++++++++++++ 5 files changed, 685 insertions(+), 67 deletions(-) create mode 100644 scripts/test_crowdin_validator.py diff --git a/README.md b/README.md index cace98c..6ee1b76 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,25 @@ 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. | + +Deleting a whole `.po` file is rejected, whether by removing the file or by +emptying it in place. Delete the individual `msgid` entries and keep the +file. `crowdin upload sources` can only ever *upload* a file, so a file +removed here would survive in Crowdin and come back — along with all its +strings — on the next pull; retiring a whole file 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..7224d55 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,60 @@ 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", + 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. + removed = capture([ + "git", "diff", "--name-only", "--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..740eb07 100755 --- a/scripts/crowdin_validator.py +++ b/scripts/crowdin_validator.py @@ -1,16 +1,35 @@ #!/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. + + Wiping out a whole file is an error too -- see removed_file() below. + Deleting strings is fine; deleting 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 +44,7 @@ import re import subprocess import sys +from collections import Counter from pathlib import Path EN_STRINGS_DIR = Path("strings/en") @@ -32,11 +52,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 +71,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 +97,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,6 +116,88 @@ 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 removed_file(path: Path, base: dict[str, str], current: dict[str, str]) -> str | None: + """Describe a `.po` that this PR wipes out entirely, or None. + + Deleting individual strings is supported; deleting the file holding them + is not, and the two are worth separating. `crowdin upload sources` can + only ever *upload* a file, so a removed file is invisible to the push: + it stays in the Crowdin project and the next pull restores it, along with + every string in it. Emptying a file in place is the same outcome by + another route, so it is caught here too -- but only for a file that had + entries to begin with, since licenses.po is legitimately empty already. + """ + if not base or current: + return None + state = "deleted" if not path.exists() else "emptied" + return f"{path} ({state}, {len(base)} string(s))" + + +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]: output = capture( ["git", "diff", "--name-only", f"origin/{BASE_BRANCH}...HEAD", "--", str(EN_STRINGS_DIR)] @@ -102,6 +214,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 +229,29 @@ def validate() -> int: return 0 existing = find_existing_keys(EN_STRINGS_DIR) - edited, deleted, collisions = [], [], [] + added, edited, deleted, renamed = [], [], [], [] + collisions, duplicates, malformed, empties, 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 = removed_file(path, base, current) + if wiped: + # Report the file once rather than every string it held. + removed.append(wiped) + 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 +260,68 @@ 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 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 (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) 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..3d152f8 --- /dev/null +++ b/scripts/test_crowdin_validator.py @@ -0,0 +1,370 @@ +#!/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) + + 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() From 6be12c89f8a61c6f179ea0845f05246257aee7ee Mon Sep 17 00:00:00 2001 From: Anthony Raj Date: Tue, 25 Aug 2026 11:31:08 -0500 Subject: [PATCH 2/4] fix(crowdin_validator): catch catalogs removed by rename changed_po_files() diffed with git's default rename detection, which reports only the destination path for a `git mv`. Renaming a whole .po therefore hid the vanished source from removed_file(): the rename passed as a pile of "added" strings, exactly the whole-file removal the check exists to reject. The push made it worse. It uploaded the destination and left the original in Crowdin -- warn_deleted_files() filtered on D, which a rename never produces -- so the next pull restored both catalogs and every string existed twice, in two files, colliding. Pass --no-renames in all three diffs. A rename is then a delete plus an add, which is what it is as far as Crowdin is concerned: there is no rename operation on the push side, only an upload of the new path. The removal surfaces normally, and so does a move *out* of strings/en/, which rename detection otherwise hid from the path-scoped diff. crowdin_sync.py's upload filter drops R along with it, since --no-renames can no longer produce one. Caught in review of #21. --- scripts/crowdin_sync.py | 8 ++++++-- scripts/crowdin_validator.py | 14 +++++++++++++- scripts/test_crowdin_validator.py | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/scripts/crowdin_sync.py b/scripts/crowdin_sync.py index 7224d55..6cd9c03 100755 --- a/scripts/crowdin_sync.py +++ b/scripts/crowdin_sync.py @@ -238,7 +238,7 @@ def changed_po_files(): parent_ref = parent.stdout.strip() diff = capture([ - "git", "diff", "--name-only", "--diff-filter=ACMR", + "git", "diff", "--name-only", "--no-renames", "--diff-filter=ACM", parent_ref, "HEAD", "--", "strings/en/*.po", ]) warn_deleted_files(parent_ref) @@ -255,8 +255,12 @@ def warn_deleted_files(parent_ref): # 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", "--diff-filter=D", + "git", "diff", "--name-only", "--no-renames", "--diff-filter=D", parent_ref, "HEAD", "--", "strings/en/*.po", ]) for path in removed.splitlines(): diff --git a/scripts/crowdin_validator.py b/scripts/crowdin_validator.py index 740eb07..4bc2a96 100755 --- a/scripts/crowdin_validator.py +++ b/scripts/crowdin_validator.py @@ -165,6 +165,8 @@ def removed_file(path: Path, base: dict[str, str], current: dict[str, str]) -> s every string in it. Emptying a file in place is the same outcome by another route, so it is caught here too -- but only for a file that had entries to begin with, since licenses.po is legitimately empty already. + Renaming or moving a catalog lands here as well, via changed_po_files()'s + --no-renames: the destination is a normal add, the source a removal. """ if not base or current: return None @@ -199,8 +201,18 @@ def detect_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")] diff --git a/scripts/test_crowdin_validator.py b/scripts/test_crowdin_validator.py index 3d152f8..0091a03 100644 --- a/scripts/test_crowdin_validator.py +++ b/scripts/test_crowdin_validator.py @@ -328,6 +328,28 @@ def test_deleting_a_whole_file_fails(self): self.assertIn("deleted, 2 string(s)", stdout) self.assertIn("1 file(s) removed", 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) removed", stdout) + self.assertIn("strings/en/app.po (deleted, 2 string(s))", stdout) + self.assertIn("2 added", 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()) + self.assertIn("whole file(s) removed", stdout) + self.assertIn("strings/en/app.po (deleted, 2 string(s))", 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) From a8f5ca326e06f11e43e16d6a2a5c9cf126997f36 Mon Sep 17 00:00:00 2001 From: Anthony Raj Date: Tue, 25 Aug 2026 11:36:01 -0500 Subject: [PATCH 3/4] feat(crowdin_validator): name the file that a catalog was renamed to --no-renames made a renamed catalog visible, but flattened it into the same "deleted" message as a genuine removal. The two need different fixes -- a rename strands the old file in Crowdin *and* uploads the new one, so the pull brings back both and duplicates every string, while a delete merely comes back -- so the report now says which happened, and where the file went. renamed_po_files() is a second, reporting-only diff with rename detection back on. It is deliberately unscoped: a move out of strings/en/ shows up as a plain delete under the path-scoped diff, because the destination no longer matches the pathspec, and naming the destination is the whole point. changed_po_files() keeps --no-renames, so correctness still rests on the decomposed delete-plus-add. strings/en/movies.po -> strings/en/films.po (12 string(s)) strings/en/fonts.po -> strings/archive/fonts.po (4 string(s)) strings/en/discover.po (deleted, 2 string(s)) strings/en/plans.po (emptied, 1 string(s)) removed_file() becomes missing_file() and returns the kind alongside the detail. Both kinds still fail the build. Also fixes shadowing this surfaced: the per-file string-rename list reused the name `renames`, clobbering the file-level rename map from the previous iteration and crashing on the second changed file. Caught by the new mixed-case test; the file-level map is now `file_renames`. --- scripts/crowdin_validator.py | 83 ++++++++++++++++++++++++------- scripts/test_crowdin_validator.py | 34 +++++++++++-- 2 files changed, 94 insertions(+), 23 deletions(-) diff --git a/scripts/crowdin_validator.py b/scripts/crowdin_validator.py index 4bc2a96..0c3c97d 100755 --- a/scripts/crowdin_validator.py +++ b/scripts/crowdin_validator.py @@ -25,8 +25,9 @@ is almost always a copy-paste slip, so it is worth a deliberate second look rather than a silent merge. - Wiping out a whole file is an error too -- see removed_file() below. - Deleting strings is fine; deleting the file that holds them is not. + 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. @@ -155,23 +156,57 @@ def empty_value_keys(entries: dict[str, str]) -> list[str]: ) -def removed_file(path: Path, base: dict[str, str], current: dict[str, str]) -> str | None: - """Describe a `.po` that this PR wipes out entirely, or None. +def renamed_po_files() -> dict[Path, Path]: + """{old path: new path} for every `.po` git sees as renamed or moved. - Deleting individual strings is supported; deleting the file holding them - is not, and the two are worth separating. `crowdin upload sources` can - only ever *upload* a file, so a removed file is invisible to the push: - it stays in the Crowdin project and the next pull restores it, along with - every string in it. Emptying a file in place is the same outcome by - another route, so it is caught here too -- but only for a file that had - entries to begin with, since licenses.po is legitimately empty already. - Renaming or moving a catalog lands here as well, via changed_po_files()'s - --no-renames: the destination is a normal add, the source a removal. + 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 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 wipes out as ("moved"|"removed", detail). + + Deleting individual strings is supported; losing the file holding them is + not, and the two are worth separating. `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 three routes there 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 -- caught only for a file + that had entries to begin with, since licenses.po is legitimately empty. """ if not base or current: return None + destination = renames.get(path) + if destination: + return ("moved", f"{path} -> {destination} ({len(base)} string(s))") state = "deleted" if not path.exists() else "emptied" - return f"{path} ({state}, {len(base)} string(s))" + return ("removed", f"{path} ({state}, {len(base)} string(s))") def entry_text(body: str) -> str: @@ -241,18 +276,21 @@ def validate() -> int: return 0 existing = find_existing_keys(EN_STRINGS_DIR) + file_renames = renamed_po_files() added, edited, deleted, renamed = [], [], [], [] - collisions, duplicates, malformed, empties, removed = [], [], [], [], [] + 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(content) - wiped = removed_file(path, base, current) + wiped = missing_file(path, base, current, file_renames) if wiped: # Report the file once rather than every string it held. - removed.append(wiped) + kind, detail = wiped + (moved if kind == "moved" else removed).append(detail) continue unparseable = malformed_keys(content) @@ -295,6 +333,8 @@ def validate() -> int: 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) @@ -316,9 +356,16 @@ def validate() -> int: renamed, ) - if not (removed or collisions or duplicates or malformed or empties): + if not (moved or removed or collisions or duplicates or malformed or empties): return 0 + 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 " diff --git a/scripts/test_crowdin_validator.py b/scripts/test_crowdin_validator.py index 0091a03..79cf6b2 100644 --- a/scripts/test_crowdin_validator.py +++ b/scripts/test_crowdin_validator.py @@ -327,6 +327,8 @@ def test_deleting_a_whole_file_fails(self): 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 @@ -337,9 +339,13 @@ def test_renaming_a_whole_file_fails(self): 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) removed", stdout) - self.assertIn("strings/en/app.po (deleted, 2 string(s))", stdout) - self.assertIn("2 added", stdout) + 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"))) @@ -347,8 +353,26 @@ def test_moving_a_file_out_of_the_strings_dir_fails(self): self.git("mv", "strings/en/app.po", "strings/archive/app.po") self.commit("archive catalog") stdout = self.assertFails(self.run_validator()) - self.assertIn("whole file(s) removed", stdout) - self.assertIn("strings/en/app.po (deleted, 2 string(s))", stdout) + # 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_emptying_a_file_in_place_fails(self): self.commit_base(app=po(entry("about", "About"), entry("bible", "Bible"))) From 3d2d3c5b8c7a79825cf9da8adc3076661d623af6 Mon Sep 17 00:00:00 2001 From: Anthony Raj Date: Tue, 25 Aug 2026 11:39:42 -0500 Subject: [PATCH 4/4] fix(crowdin_validator): reject losing a catalog that held no strings missing_file() keyed off entries lost, so a .po with none looked like nothing to lose: `git rm strings/en/licenses.po` passed with exit 0, and renaming it passed too. licenses.po is empty but is still a tracked file, and the rule is about the file, not its contents -- a catalog on the base branch has to still be there whether it held 600 strings or none. Test with `git cat-file -e` against the base ref instead. base_content() returning "" cannot distinguish "not tracked" from "tracked but empty", which is the ambiguity that opened the hole. A path absent from the base branch is still exempt, so adding a new catalog stays fine. Emptying a file in place is the one variant that still turns on contents: it only counts as a loss if there were entries to lose. README states the rule outright -- never delete, rename or move a .po. --- README.md | 16 ++++++---- scripts/crowdin_validator.py | 50 +++++++++++++++++++++---------- scripts/test_crowdin_validator.py | 29 ++++++++++++++++++ 3 files changed, 75 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 6ee1b76..601ee0e 100644 --- a/README.md +++ b/README.md @@ -52,9 +52,15 @@ reports but does not block them: | 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. | -Deleting a whole `.po` file is rejected, whether by removing the file or by -emptying it in place. Delete the individual `msgid` entries and keep the -file. `crowdin upload sources` can only ever *upload* a file, so a file -removed here would survive in Crowdin and come back — along with all its -strings — on the next pull; retiring a whole file has to start in the +**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/scripts/crowdin_validator.py b/scripts/crowdin_validator.py index 0c3c97d..60bac8e 100755 --- a/scripts/crowdin_validator.py +++ b/scripts/crowdin_validator.py @@ -179,34 +179,54 @@ def renamed_po_files() -> dict[Path, Path]: 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 wipes out as ("moved"|"removed", detail). + """Classify a `.po` this PR loses as ("moved"|"removed", detail). Deleting individual strings is supported; losing the file holding them is - not, and the two are worth separating. `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 three routes there 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 -- caught only for a file - that had entries to begin with, since licenses.po is legitimately empty. + 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 base or current: + if not exists_in_base(path): return None destination = renames.get(path) if destination: return ("moved", f"{path} -> {destination} ({len(base)} string(s))") - state = "deleted" if not path.exists() else "emptied" - return ("removed", f"{path} ({state}, {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: diff --git a/scripts/test_crowdin_validator.py b/scripts/test_crowdin_validator.py index 79cf6b2..b95edd0 100644 --- a/scripts/test_crowdin_validator.py +++ b/scripts/test_crowdin_validator.py @@ -374,6 +374,35 @@ def test_renaming_one_file_while_deleting_another_reports_each_kind(self): 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)