Skip to content

feat(crowdin): allow source string edits and deletes - #21

Merged
anthonyraj merged 4 commits into
mainfrom
air/fix-scripts-to-allow-edit-deltes
Aug 25, 2026
Merged

feat(crowdin): allow source string edits and deletes#21
anthonyraj merged 4 commits into
mainfrom
air/fix-scripts-to-allow-edit-deltes

Conversation

@anthonyraj

@anthonyraj anthonyraj commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

What

scripts/crowdin_validator.py 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. This makes the repo a legitimate origin for all three.

Findings now split in two:

Outcome
NOTE — added, edited, deleted, renamed reported with old → new text, exit 0
ERROR — whole file removed, cross-file duplicate key, key defined twice in one file, unparseable entry, empty value exit 1

Structural checks are scoped to the files a PR changed, so a pre-existing quirk elsewhere can't fail an unrelated PR.

Deleting strings is allowed; deleting the file that holds them is not. 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. Emptying a file in place is the same outcome by another route and is caught the same way — but 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 per-string delete notices.

Renames are detected by pairing a deleted key with an added key carrying identical text, and called out separately: that's the one delta where Crowdin's key matching means translations don't survive.

Two coupled fixes, without which the above would be cosmetic

  • crowdin/crowdin.yml gains update_option: update_as_unapproved. These files carry X-Crowdin-SourceKey: msgstr, so editing a msgstr is a source-text change. Crowdin's default would have discarded that string's translations in every language. Validated against the CLI's config parser, with a deliberately-invalid control to confirm the check actually runs.
  • 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.

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 — 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.

Also softened: the cross-file duplicate-key message no longer claims Crowdin rejects them. Crowdin scopes keys per file and tolerates it — 53 such pairs already exist on main (about, back, done, …). It still fails the build because it's almost always a copy-paste slip, but it only ever fires on newly-added keys, so no existing content is affected.

Tests

Adds scripts/test_crowdin_validator.py — the repo's first tests. Stdlib unittest, no new dependencies.

python3 -m unittest discover -s scripts -p 'test_*.py'

36 cases: 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. Includes merge shapes (delete + edit, two-into-one, cross-file) to pin that a merge is never misread as a rename, and boundary cases for the whole-file rule.

Verification

  • 36/36 pass; all four structural checks report zero findings across the 859 real keys in strings/en/.
  • Real scratch branch, mixed delta (1 add, 1 edit, 1 delete, 1 rename) → exit 0, all classified correctly. Four deliberate mistakes → exit 1, all four errors fired.
  • Removing movies.po (12 strings) + emptying fonts.po (4 strings) → exit 1, both named with counts. Deleting two strings from app.po while keeping the file → exit 0.
  • --push --dry-run: correct per-file scoping, no --delete-obsolete, whole-file-delete warning fires.

Reviewer notes

  • ⚠️ Branch name deviates from AGENTS.md. The convention is initials/description-TICKET-ID with the ticket last; this branch has no ticket ID, and deltes is a typo for deletes. Flagged here per .claude/rules/branch-naming.md rather than silently renaming, since the name was chosen deliberately. Happy to rename if you'd prefer.
  • ⚠️ The live Crowdin round-trip is unverified — push a scratch key, delete it, push again, confirm a pull doesn't resurrect it. That needs the real CROWDIN_API_TOKEN against shared project 257. Everything else was verified offline, but the reconcile-on-upload behaviour relied on for per-string deletes is inferred from the CLI's documented --auto-update, not observed live.
  • youversion-flutter-loop/scripts/crowdin_validator.py is a near-identical copy whose validate() was byte-for-byte the same. It still blocks edits and deletes for that repo — separate PR.

Greptile Summary

The PR allows repository-originated source-string additions, edits, and deletions while retaining structural validation and preventing whole-catalog loss.

  • Adds reporting and validation for string edits, deletions, renames, malformed entries, empty values, and duplicate keys.
  • Rejects deleted, renamed, moved, or emptied catalogs and adds a sync-time warning for deleted files.
  • Preserves translations from edited source text as unapproved and adds comprehensive validator tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
scripts/crowdin_validator.py Expands validation to classify string-level changes and reliably rejects whole-catalog deletion, renaming, moving, or emptying.
scripts/test_crowdin_validator.py Adds broad parsing and end-to-end coverage, including the previously reported catalog-rename path.
scripts/crowdin_sync.py Updates push semantics and warns when deleted catalogs cannot be removed through the upload path.
crowdin/crowdin.yml Keeps translations for edited source strings while marking them unapproved for review.
README.md Documents the revised source-string workflow and the prohibition on deleting or renaming catalogs.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Repository source-string change] --> B[Catalog validator]
    B -->|Valid string-level change| C[Merge]
    B -->|Structural error or catalog loss| D[Reject PR]
    C --> E[Crowdin source upload]
    E --> F[Updated Crowdin source state]
    F --> G[Future source pull]
    G --> A
Loading

Reviews (4): Last reviewed commit: "fix(crowdin_validator): reject losing a ..." | Re-trigger Greptile

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'
@anthonyraj anthonyraj self-assigned this Aug 24, 2026
Comment thread scripts/crowdin_validator.py Outdated
Anthony Raj added 3 commits August 25, 2026 11:31
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.
--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`.
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.
@anthonyraj
anthonyraj merged commit 34357ae into main Aug 25, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant