Localization: stage translations into the String Catalog (manual)#25713
Localization: stage translations into the String Catalog (manual)#25713jkmassel wants to merge 2 commits into
Conversation
Generated by 🚫 Danger |
d5697f8 to
a45aaf1
Compare
b558570 to
c5a43d9
Compare
Adds the localize half of the String Catalog pipeline: a manual (non-CI) lane that folds current GlotPress translations for regular, non-plural strings into the existing Localizable.xcstrings as `human ?? AI ?? English`. The catalog isn't the runtime store yet (the app still ships Localizable.strings), so this only pre-populates it for the eventual cutover — nothing users see changes. - `localize_catalog` lane: download current GlotPress translations into a throwaway dir, fold them in (human => `translated`), then AI-fill the rest (=> `needs_review`). Manual only — it calls the translation API (cost) and commits a large catalog; never wired into `download_localized_strings` or CI. - `CatalogStrings.fold_translations!`: pure, reuse-aware fold. An existing valid cell is kept, so re-runs only translate genuinely-new gaps — the catalog's `needs_review` state IS the persistence (no side-store). A human translation from GlotPress supersedes a kept cell on the next fold. - Immutable-key enforcement replaces `reconcile_changed_sources`: reworded English in place is now a hard error, since `xcstringstool sync` silently keeps the old key's translations and would ship stale text. Rewording requires a new key. - `translate_all` keeps completed batches when one fails mid-run, so a partial API failure degrades to English for the remainder instead of dropping the lot. - docs/localization-pipeline.md documents the regular-string fold and the `human ?? AI ?? English` floor — any translation, human or machine, that fails the placeholder gate falls through to the next rung.
c5a43d9 to
cd9b85f
Compare
oguzkocer
left a comment
There was a problem hiding this comment.
I've completed my first pass on this and found one issue reported by Claude and opened a PR to add a couple failing tests for it: #25802
I didn't see anything in my manual review, but I also haven't spent a significant time on it yet. If it's not reviewed by anyone else, I'll pick this up again once #25802 is addressed.
…tions (#25802) * Add failing tests: placeholder gate rejects valid reordered human translations TranslationValidator compares a source's positional and sequential specifier views independently, so it only permits reordering when the source is ALREADY positional. A non-positional source (`%@ … %@`) whose translation reorders via positional specifiers (`%2$@ … %1$@`) is rejected — even though that is the standard, Apple-documented iOS reordering mechanism and is correct at runtime (`String(format:)` honors positional specifiers regardless of the source shape). The human-translation gate added for the String Catalog fold (`CatalogStrings.trusted_human`, `PluralStrings.human_forms_for`) inherits this, so it rejects and downgrades valid, currently-shipping human translations to machine/English. This is not hypothetical: the committed translated `.strings` contain 23 key-as-source strings across 34 locales (513 cells) that positionalize a bare-`%@` English source this way, e.g. ar "%@ of %@ used on your site" => "%1$@ من %2$@ على موقعك". Two failing tests pin the correct behavior: - translation_validator_test.rb — the root cause at the validator. - catalog_strings_helper_test.rb — the user-facing downgrade in the fold. The fix belongs in TranslationValidator (shared by AI, plurals, and regular human strings): treat a non-positional source as implicitly numbered 1..N by appearance order and accept a fully-positional candidate whose index→type map matches. Existing tests already keep genuine breakage (sequential flip, type/ count change) rejected, so a correct fix cannot just drop the distinction. * Fix placeholder gate: accept positional reorder of a non-positional source TranslationValidator compared a source's positional and sequential specifier views independently, so it only permitted reordering when the source was ALREADY positional. A non-positional source (`%@ … %@`) whose translation reorders via positional specifiers (`%2$@ … %1$@`) was rejected — even though that is the standard, Apple-documented iOS mechanism and is correct at runtime, because `String(format:)` honors positional specifiers regardless of the source's shape. Normalize a purely-sequential signature into the implicit 1..N positional map its bare specifiers already carry (`%@ %d` => `{1 => object, 2 => int}`) before comparing, applied to both source and candidate. Once both sides are index→type maps, `%@ - %@` and `%2$@ - %1$@` compare equal and the reorder is accepted. Genuine breakage stays rejected: a sequential flip `%@: %d` => `%d : %@` yields `{1 => object, 2 => int}` vs `{1 => int, 2 => object}` — a disagreement on which index owns which type — and type/length/count changes are unaffected. A signature that already uses positional specifiers, or one that mixes bare and positional specifiers (malformed), is left unchanged, so mixed strings still require an exact structural match. Fixes the two failing tests added in the previous commit and propagates to all three gate consumers (AI, plurals, regular human strings) via the shared validator. All 7 fastlane/lanes suites pass (113 tests); rubocop clean. * Add failing tests: gate and coverage compare miss format-argument edge cases Three currently-red tests documenting real defects found auditing the placeholder gate and the build-free coverage compare: - A candidate that binds the same positional index to two different types (%1$@ %1$d) is accepted: signature() collapses it last-wins, and this PR's implied_positional widened the reach to bare-specifier sources (%d), which regressed from reject to accept. - Dynamic field width/precision (%*d, %.*f) consume an extra int vararg the signature never counts, so %d and %*d reduce to the same signature. - coverage_gap conflates keys that differ only in argument type (%d days vs %@ days), masking a genuinely dropped key behind a same-prose sibling. * Fix placeholder gate: reject a positional index reused with a conflicting type signature() stored positional specifiers in a Hash keyed by index with plain assignment, so a second reference to the same index (`%1$@` then `%1$d`) silently overwrote the first, collapsing an object/int conflict to one token. A candidate that references argument 1 as both an object and an int passes an int to a `%@` conversion at runtime — a wrong-vararg read. This PR's implied_positional widened the reach: a bare-specifier source like `%d` now normalizes to {1 => int} and matched the collapsed candidate, regressing that case from reject to accept. Record a conflicting reuse as a distinct token so it can never equal a well-formed source's single type, while still allowing a consistent reuse (`%1$d … %1$d`, which legally reads one argument twice). * Fix placeholder gate: count dynamic field width/precision arguments A `*` in a specifier's field width or precision (`%*d`, `%.*f`) consumes its own int argument before the value, so `%d` and `%*d` are not interchangeable. The FORMAT_SPECIFIER regex matched the `*` but signature() emitted no token for it, so the two reduced to the same signature and a translation could add or drop a dynamic-width/precision argument undetected — reading one vararg past what the call site supplies. Emit an int argument for each `*`, in appearance order before the value. * Fix coverage compare: keep argument type when canonicalizing keys canonical() replaced every format specifier with one sentinel regardless of type, so keys differing only in argument type (`%d days` vs `%@ days`) collapsed to the same coverage cell — a genuinely-dropped key was masked by a same-prose sibling of a different type, defeating the same-basename overwrite regression the coverage gate exists to catch. Strip only the positional index (the sole difference between the genstrings source-form `%li` and the xcstringstool-normalized `%1$li` for the same source literal), so `%li` and `%1$li` still compare equal while `%d` and `%@` stay distinct. * Satisfy RuboCop: extract signature helpers and merge the catalog test class Danger's RuboCop pass flagged two offenses from the previous commits: - Metrics/AbcSize: signature() exceeded the ABC limit once it gained the positional-conflict and dynamic-width handling. Extract record_specifier and add_positional so signature() is a thin loop again; behavior unchanged. - Style/OneClassPerFile: the coverage_gap test added a second top-level class to catalog_helper_test.rb. Fold it into the file's single test class, renamed CatalogHelperTest since it now covers both reword detection and the coverage compare. --------- Co-authored-by: Jeremy Massel <1123407+jkmassel@users.noreply.github.com>
|
@oguzkocer – I fixed that issue (and a couple of related ones) in #25802 then landed it into this PR. |
| # --- Failing tests: gaps surfaced by the pipeline-breaker audit (currently RED; each documents a real defect | ||
| # where the gate violates its own "same count, same types, same index→type mapping" invariant) ----------- | ||
|
|
There was a problem hiding this comment.
These are now green, I think. https://buildkite.com/automattic/wordpress-ios/builds/33234/canvas?jid=019f778c-7142-4c96-8103-a67d788ee4c4&tab=output
mokagio
left a comment
There was a problem hiding this comment.
I asked Opus 4.8 for help in reviewing this PR. Of the various points it raised, the one that stood out to me regards the consequences of a no-API-key run.
Implicit tradeoff: the fold materializes a cell for every
(key, locale)pair — 141,240 cells, ~22 MB committed — and the "still English" count in the run summary is derived from those materialized cells rather than computed. So the storage decision and the reporting decision are silently coupled, and nothing in the PR says so.Where it bites today: the no-API-key run the PR recommends as the cheap plumbing check writes 141,240 English-fallback cells and commits 21.5 MB of zero-information JSON. In the intended mode (AI on) this is a non-issue — English fallbacks drop to zero.
Suggestion: don't commit when the AI tier is disabled (or warn loudly), and make
summarizederiveenglish:astranslatable_total − human − machineso a future prune of English-fallback cells doesn't silently zero out that number.
ai_translator = catalog_ai_translator
written = CatalogStrings.fold_translations!(catalog, ..., ai_translator: ai_translator)
...
if ai_translator.nil? && options[:commit].to_s != 'true'
UI.important('Skipping the commit: no AI tier, so every gap cell is an English placeholder. ' \
'Re-run with ANTHROPIC_API_KEY set, or pass commit:true to stage the human-only fold.')
next
end
git_add(path: LOCALIZABLE_CATALOG, shell_escape: false, force: true)
git_commit(...)
Fills the app's String Catalog (
Localizable.xcstrings) with translations — the human translations we already have from GlotPress, plus machine translations for whatever is still untranslated. This gets the catalog ready ahead of moving the app over to String Catalogs. Nothing users see changes.Two things to know
Staged, not shipped. The catalog file is generated, but the app doesn't read it at runtime yet — regular strings still come from the existing
Localizable.strings. This only pre-fills the catalog for the eventual switch-over. Machine translations are markedneeds_review, so they stay clearly separate from reviewed human translations and can sit in the catalog without shipping to anyone.Manual, not automated. These lanes are run by hand, not wired into any release or CI step: a run extracts strings, calls a paid translation API, and commits a large file, so it shouldn't happen on its own. The unit tests still run in CI.
How it works
Three steps, each runnable on its own:
generate_strings_catalog— extracts the English strings from the code into the catalog.download_catalog_strings— downloads the current translations from GlotPress.localize_catalog— adds the human translations, machine-translates whatever's left, and commits.Each string gets the best translation available in each language: a human translation if there is one, otherwise a machine translation, otherwise the English text. Human translations are marked
translated; machine translations and English fallbacks are markedneeds_review.Re-running is cheap. A machine translation already in the catalog is left alone instead of being requested again, and a human translation always replaces a machine one on the next run. Machine translation only happens when an API key is set — without one, the catalog fills from human translations and English. A machine translation is kept only if its format placeholders (
%1$@,%2$d, …) match the English, so a mangled one falls back to English rather than shipping a broken string.Test plan
fastlane/lanes/*_test.rb) — these run in CIrubocopcleanManual run
First
bundle install. A machine-translation run also needs anANTHROPIC_API_KEY— the Fastfile picks it up from~/.wpios-env.default. Without a key the run still works: it fills in the human translations and English only, skips machine translation, and costs nothing — a good way to check the plumbing before spending anything.Run the three lanes, scoped to one well-translated locale to keep it cheap:
generate_strings_catalogextracts the English strings intoLocalizable.xcstrings(~3,900 of them). No network, no cost.download_catalog_stringsdownloads the French translations from GlotPress into a scratch folder. No cost.localize_catalogfills the French translations into the catalog, machine-translates the gaps, and commits.French is well-translated, so most entries come from GlotPress and only a small number fall to machine translation, which keeps the cost for one locale low. A few entries may stay English where a machine translation was rejected for not matching the English placeholders.
localize_catalogprints a per-locale summary at the end, so you can check the result straight from the log — no need to open the catalog:Expect mostly human, a smaller machine count, and only a few still English. The sample lines let you spot-check the machine translations for obvious nonsense.
localize_catalogcommits the catalog, which is a large file. This run is only to check the output and cost, so discard the commit rather than pushing it (git reset --hard HEAD~1). The catalog gets committed for real during the migration, not in this PR.Plurals are handled the same way in a separate PR (#25710), since they live in their own catalog file.