From ebd3a68e3b1c83959925de3bf32ad8c222fde4c8 Mon Sep 17 00:00:00 2001 From: davotoula Date: Fri, 14 Aug 2026 08:13:43 +0200 Subject: [PATCH] docs: teach the translation skill the lint gate it was missing A translation pass last night cleared every check the find-missing-translations skill prescribes - no duplicate keys, well-formed XML, a green convertXmlValueResourcesForCommonMain, a green compileFdroidDebugKotlin - and still took CI red with three lint errors. The plural rules live in Android lint, not in the resource compiler, and the skill never ran it. Six additions, each from a failure in that pass: - Step 6 now runs :amethyst:lintPlayBenchmark and reads the SARIF for zero errors, with a table of the rules that gate: MissingQuantity and ImpliedQuantity are errors, StringFormat* are warnings, and there is no lint baseline so abortOnError bites on the first one. - Converting a to needs every locale's full CLDR category set. The "use other only, Crowdin fills the rest" shortcut fails MissingQuantity before any sync happens. res/CLAUDE.md step 3 advised exactly that shortcut, so it is corrected here too, and the declension trap is called out - the retained text is the plural form, so reusing it for "one" yields "1 odpowiedzi". - tools:ignore belongs on the source entry, never a locale file. Crowdin propagates source attributes into its exports; an attribute added only to values-xx is absent from the next one. The tools:ignore="Typos" copies in cs/de/ar/eo/bn are the result of that propagation, not evidence that locale attributes survive - mistaking one for the other is what broke main. - A new format-specifier parity and empty-item audit, for the class where the key is present and looks translated but the placeholder was dropped or escaped. The (? Claude-Session: https://claude.ai/code/session_012uQksy5spXR8gC8Z5QfsRB --- .../skills/find-missing-translations/SKILL.md | 152 ++++++++++++++++++ amethyst/src/main/res/CLAUDE.md | 2 +- 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/.claude/skills/find-missing-translations/SKILL.md b/.claude/skills/find-missing-translations/SKILL.md index ef7d1c5e40..491b2478ef 100644 --- a/.claude/skills/find-missing-translations/SKILL.md +++ b/.claude/skills/find-missing-translations/SKILL.md @@ -77,6 +77,25 @@ What this means for this skill: 2. **Source-identical entries are a small, recognizable minority.** Brand terms (`Nowhere X`), single-word loanwords (`Apps` / `Feed` / `Issues`), and bare version/format strings (`v%1$s`) are the usual cases. Skip these by inspection rather than translating them to something identical. 3. **Don't add source-identical fallbacks.** Android falls back to `values/strings.xml` at runtime, so a key intentionally kept as English already renders correctly, and Crowdin's next sync would strip a local duplicate anyway. +4. **A repo-side edit to a translated value only sticks where Crowdin's database + doesn't contradict it.** Download replaces file content with Crowdin's current + export; it does not diff or merge. So a hand fix to a locale file survives only + if Crowdin happens to hold the same value (or holds nothing for that key). If + Crowdin holds a *different* value — including an **empty** one — the next sync + silently reverts you. + + Observed 2026-08-13/14 in one pass, which is what makes the rule concrete: + `pow_estimate_minutes[few]` (pl) **survived** the sync because Crowdin's + approved value matched the fix, while `nest_listener_count[many]` (pl) was + **reverted to empty** two commits later because Crowdin stores an empty string + there. Same file, same commit, opposite outcomes. + + Consequences: fixing a *value* durably means entering it in the Crowdin web UI + — no repo commit will hold it. Changes to the **source** file are different and + do stick, because that file is Crowdin's input, not its output: deleting a key + from `values/strings.xml` removes it project-wide, and attributes declared + there propagate into every export. + > **Historical note:** an earlier version of this skill tried to auto-filter the > candidate list with a git "sync-timestamp" heuristic (skip any key added before > the last `New Crowdin translations` commit). It was **dropped** because it @@ -280,6 +299,64 @@ done For each hit, warn the user that the entry is unreachable in that locale. The fix is to **remove the ``** and, if the UX wanted distinct wording for count=0, add a separate `` plus an `if (count == 0)` branch at the call site (see "Plurals: handle with care" below). +Also audit **format-specifier parity and empty items** across the locales you +touched. These are a different defect class from a missing key — the key is +present and looks translated, but the placeholder was dropped, escaped, or the +item left blank, so the number never reaches the user: + +```bash +python3 - <<'PY' +import re, io, glob +keyre = re.compile(r']*>(.*?)', re.S) +plre = re.compile(r']*>(.*?)', re.S) +itre = re.compile(r']*>(.*?)', re.S) +# (?' \ + amethyst/src/main/res/values*/strings.xml \ + commons/src/commonMain/composeResources/values*/strings.xml +``` + +Three things this scan taught us, all of which it now encodes: + +- **The `(?`** entries, follow these rules: pluralStringResource(R.plurals.foo_items, count, dateLabel, count) } ``` +- **Converting an existing `` to ``: give every locale its FULL + category set, not just `other`.** You must convert it in every locale that + already had the `` (aapt2 rejects a resource-type mismatch across + locales, and an orphaned locale `` trips `ExtraTranslation`) — but + carrying the old text across as an `other`-only block, on the theory that + Crowdin backfills the rest, **fails `MissingQuantity` and breaks CI before + Crowdin ever gets a turn.** Supply `one`/`few`/`many` for pl, `one` for hu, and + so on, at conversion time. + + Note this contradicts `amethyst/src/main/res/CLAUDE.md` step 3, which still + advises the `other`-only shortcut. That advice is wrong; prefer this. + + Watch the declension when you do it: the retained text is usually the *plural* + form, so reusing it verbatim for `one` produces "1 odpowiedzi". (2026-08-13: + converting `poll_results_selections` with `other` only errored on both hu and + pl, and the retained pl text was the few/many form.) + +- **A `tools:ignore` suppression must go on the SOURCE entry in + `values/strings.xml`, never on a locale file.** Crowdin propagates attributes + declared on the source into every translation it exports; an attribute you add + to `values-xx/strings.xml` alone is simply absent from the next export. That is + why the existing `tools:ignore="Typos"` entries survive — they are declared on + the source, and the copies in cs/de/ar/eo/bn are the *result* of propagation, + not evidence that locale-file attributes stick. (2026-08-13: an + `ImpliedQuantity` suppression added only to `values-pt-rBR` was stripped by the + next sync and took `main`'s CI red.) + + Before reaching for a suppression at all, check whether the key is even used — + a `grep -rn "" --include='*.kt'` that returns nothing means deleting the + key is the better fix than muting the rule that objects to it. + - Reference: [Android `` docs](https://developer.android.com/guide/topics/resources/string-resource#Plurals) and [CLDR plural rules](https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html). **Then ask the user:** "Would you like me to translate these missing strings into [list of target locales]?" @@ -380,6 +488,44 @@ When adding translated strings to locale files: # ./gradlew :commons:convertXmlValueResourcesForCommonMain ``` +- **Then run Android lint. This is the gate that actually matches CI, and the + checks above do NOT substitute for it.** Duplicate-key + well-formedness + + `convertXmlValueResourcesForCommonMain` can all pass on a change that still + takes CI red, because the plural rules live in lint, not in the resource + compiler: + + ```bash + ./gradlew :amethyst:lintPlayBenchmark # the task CI runs (.github/workflows/build.yml) + ``` + + There is no `lint-baseline.xml` in this repo and only `MissingTranslation` is + disabled (`amethyst/build.gradle.kts`), so `abortOnError` bites on the first + error. Three rules matter for a translation pass: + + | Rule | Fires when | Severity | + |------|-----------|----------| + | `MissingQuantity` | a locale's `` omits a CLDR category that locale uses | **error** for core categories — gates CI | + | `ImpliedQuantity` | a `quantity` item has no format argument in a locale where that category spans more than one number | **error** — gates CI | + | `StringFormatCount` / `StringFormatMatches` | a translation's placeholder count/type disagrees with the base entry | warning | + + (2026-08-13: a pass that cleared the duplicate/XML gate above still failed + `lintPlayBenchmark` with 3 errors. Compiling is not evidence — `compileDebugKotlin` + passed on the same change.) + +- **Confirm the report says zero errors, don't just trust BUILD SUCCESSFUL** of a + wider invocation: + + ```bash + python3 -c " + import json,io,collections + d=json.load(io.open('amethyst/build/reports/lint-results-playBenchmark.sarif',encoding='utf-8')) + r=d['runs'][0]['results'] + print(dict(collections.Counter(x.get('level','warning') for x in r))) + for x in r: + if x.get('level')=='error': print('ERROR', x['ruleId'], x['locations'][0]['physicalLocation']['artifactLocation']['uri']) + " + ``` + ## Common Mistakes - **Scanning only the amethyst tree** — there are now **two** Crowdin-managed `strings.xml` trees (`amethyst/src/main/res` and `commons/src/commonMain/composeResources`). A key extracted into `commons/` will never show up in the amethyst diff. Run the whole technique once per tree (see "Resource trees") and report each separately. @@ -392,6 +538,12 @@ When adding translated strings to locale files: - **Adding source-identical fallbacks locally** — they get overwritten on the next Crowdin sync. Android falls back to `values/strings.xml` at runtime anyway, so a key intentionally kept as English already renders correctly. Skip these by inspection (brand terms, loanwords, `v%1$s`-style strings); don't translate them to an identical value. - **Skipping per-locale diffs when only diffing cs** — Crowdin can strip different keys in different locales (each translator's choice), so cs is not a reliable upper bound. Diff each target locale and union the results. - **Pasting the union set of missing keys into every locale → duplicate keys** — the union is the right set to *translate*, but the wrong set to *insert*. A key missing in only some locales, inserted into all of them, duplicates in the ones that already had it. Drive each file's insertion off its own per-locale diff (see Step 6). In `commons`, a duplicate key is build-breaking: `convertXmlValueResourcesForCommonMain` fails with `Duplicated key '…'`. **Always run the post-insertion duplicate + XML-wellformedness gate in Step 6 before declaring done.** (Happened 2026-07-21 with `ps1_save_block` / `podcast_value_for_value` / `chats_history_relays`.) +- **Declaring the pass done without running `:amethyst:lintPlayBenchmark`** — the duplicate-key + XML + `convertXmlValueResourcesForCommonMain` gate is necessary but nowhere near sufficient. `MissingQuantity` and `ImpliedQuantity` are errors, there is no lint baseline, and `abortOnError` is on, so a change that compiles and passes every check in Step 6's first half can still take CI red. Compiling is not evidence. (Happened 2026-08-13: 3 lint errors after a clean duplicate/XML gate and a green `compileFdroidDebugKotlin`.) +- **Converting a `` to `` with `other` only** — "Crowdin fills the rest" is false; `MissingQuantity` errors immediately and CI fails before any sync. Supply every category the locale uses at conversion time, and re-check the declension rather than reusing the old text for `one`. +- **Putting `tools:ignore` on a locale file** — Crowdin strips it on the next export. Suppressions belong on the source entry in `values/strings.xml`, which propagates. The `tools:ignore="Typos"` copies visible in cs/de/ar/eo/bn are the *result* of that propagation, not proof that locale-file attributes survive. (Happened 2026-08-13; it broke `main`.) +- **Suppressing a lint rule on a key nothing references** — check `grep -rn "" --include='*.kt'` first. `poll_results_voters` was a bare noun with no count, zero call sites, and an unlocalizable shape; deleting it retired the problem outright where a suppression would only have muted it. +- **Comparing placeholders without a `(?` as merely "untranslated"** — it renders as nothing at runtime, and for a category like Polish `many` (5–21, 25–31, …) that is the common case, not an edge case. Grep for them explicitly; the missing-key diff will never surface one because the key is present. - **Inserting strings in a specific position** — always append at the bottom; ordering is handled separately - **Hardcoding `"1"` in a `` `quantity="one"` item** — always use the count placeholder; otherwise non-English `one` categories produce wrong text - **Copying English's `one`/`other` set into every locale** — each language must include all CLDR plural categories it uses (e.g. Czech needs `one`, `few`, `many`, `other`) diff --git a/amethyst/src/main/res/CLAUDE.md b/amethyst/src/main/res/CLAUDE.md index e7d124f6bb..26fb7dc71e 100644 --- a/amethyst/src/main/res/CLAUDE.md +++ b/amethyst/src/main/res/CLAUDE.md @@ -51,5 +51,5 @@ Defined in `amethyst/src/main/java/com/vitorpamplona/amethyst/ui/StringResourceC 1. Add the new `` to default `values/strings.xml` with `one` + `other`. 2. If you're also adding a locale-specific translation (e.g. zh-rCN, pl-rPL), add it as `` with at least `other`. Crowdin will fan out to all CLDR categories for that locale. -3. If you're **converting** an existing `` to ``, you **must** convert it in every locale that already had the `` — otherwise aapt2 will fail with a resource-type mismatch. Use `` with `other` only to preserve existing translation (Crowdin fills the rest). +3. If you're **converting** an existing `` to ``, you **must** convert it in every locale that already had the `` — otherwise aapt2 will fail with a resource-type mismatch, and an orphaned locale `` trips `ExtraTranslation`. Give each locale its **full CLDR category set** at conversion time. Carrying the old text over as an `other`-only block does **not** work: lint's `MissingQuantity` is an error, so CI fails long before Crowdin gets a chance to backfill. Mind the declension too — the retained text is usually the plural form, so reusing it verbatim for `one` yields "1 odpowiedzi". 4. Reference: [Android `` docs](https://developer.android.com/guide/topics/resources/string-resource#Plurals) and [CLDR plural rules](https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html).