Skip to content

Add ios_lint_localization_placeholder_changes action#738

Draft
jkmassel wants to merge 3 commits into
trunkfrom
jkmassel/issue-736
Draft

Add ios_lint_localization_placeholder_changes action#738
jkmassel wants to merge 3 commits into
trunkfrom
jkmassel/issue-736

Conversation

@jkmassel

Copy link
Copy Markdown
Contributor

What does it do?

Adds a placeholder-compatibility guardrail for localization source strings, as requested in #736.

Existing translations are keyed by a string's key, not its English text. If an existing key's source value changes its format placeholders — count, position, or argument type — every translation filed under that key silently breaks (or crashes) at the new call site. This is a different axis from ios_lint_localizations, which compares each translation against the base language at a single point in time; this compares base ↔ base across versions (temporal).

1. Reusable primitive — Fastlane::Helper::StringPlaceholdersHelper

Pure Ruby, no file I/O, platform-agnostic (the %@ / %1$d placeholder syntax is shared by iOS and Android):

  • placeholder_signature(value) — canonical signature of a value's placeholders ('' if none), e.g. "1:int,2:object".
  • placeholders_compatible?(a, b)signature(a) == signature(b).
  • incompatible_placeholder_changes(old_hash, new_hash) — keys present in both whose signature differs, each { key:, old:, new:, old_signature:, new_signature: }. New/removed keys are ignored on purpose: copy that needs a fresh translation is expected to land under a new key.

The signature is invariant to benign copy edits and to reordering equivalent positional args, but sensitive to count/position/type changes. %d%i are compatible (both int); %d%@ are not. %% is skipped; width/precision/length modifiers are ignored for type-classing.

2. iOS action — ios_lint_localization_placeholder_changes

Takes old_file / new_file (the previous and newly-generated base .strings), parses them via the existing L10nHelper.read_strings_file_as_hash (plutil), runs the primitive, and reports. Mirrors ios_lint_localizations: abort_on_violations (default true) and allow_retry (interactive retry loop).

Worked examples (match the issue spec exactly — verified by tests)

Value Signature
Just text (empty)
Hello %@ 1:object
%1$d items in %2$@ 1:int,2:object
%2$@ told by %1$@ 1:object,2:object
100%% sure about %@ 1:object
%d likes 1:int

Notes / scope

  • Android wrapper deferred to a follow-up, as the issue permits ("Android wrapper (or a follow-up issue)"). strings.xml parsing (Nokogiri, plurals, string-arrays, distinct escaping rules) is non-trivial and would roughly double this diff. The primitive is already platform-agnostic, so the follow-up is just a thin android_* parse-and-call wrapper.
  • Reuses L10nHelper.read_strings_file_as_hash rather than adding a second .strings parser — the action is therefore macOS-only by design, like ios_lint_localizations.
  • .stringsdict / ICU plurals are out of scope (open question in the issue; likely v2).
  • No MIGRATION.md entry — purely additive.
  • Heads-up: the diff is ~544 added lines vs Danger's 500-line guideline (275 of those are tests). Happy to trim the docs/heredocs if reviewers prefer a clean Danger run.

Test Plan

  • bundle exec rubocop — no violations (full repo, 204 files).
  • bundle exec rspec — 36 new examples pass (30 helper + 6 action); rest of the suite green. The one failing git_helper lfs test is environmental (git-lfs not installed locally) and unrelated to this change.
  • Helper specs cover every worked example + edge cases (%%, nil/whitespace, non-ASCII, escapes, mixed positional/non-positional, width/precision/length modifiers, %d%i).
  • Action specs write real .strings fixtures parsed by plutil: no-op, abort, abort_on_violations: false, add/remove ignored, retry-after-fix, missing file.

Related issues

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations.
  • Add Unit Tests (aka specs/*_spec.rb) if applicable.
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass.
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the MIGRATION.md file — N/A, purely additive.

@dangermattic

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@jkmassel jkmassel force-pushed the jkmassel/issue-736 branch from e329223 to cbbfbfd Compare June 20, 2026 00:34
Adds a platform-agnostic `StringPlaceholdersHelper` primitive
(`placeholder_signature`, `placeholders_compatible?`,
`incompatible_placeholder_changes`) and an iOS action that fails when an
existing localization key's source-language value changes its format
placeholders — count, position, or argument type — between two versions of the
base `.strings` file. Such a change silently breaks every existing translation
filed under that key. Complements `ios_lint_localizations` on the temporal
(base↔base across versions) axis.

The action aborts (by default, via `check_duplicate_keys`) when either input
file defines a key more than once, since `plutil` keeps only the last value and
a duplicate could otherwise hide a real change; and it reports a clean error
when an input file cannot be parsed. As part of the duplicate-key check,
`StringsFileValidationHelper.find_duplicated_keys` now also handles unquoted
keys (valid `.strings` syntax, common in `InfoPlist.strings`).

Part of #736.
@jkmassel jkmassel force-pushed the jkmassel/issue-736 branch from cbbfbfd to d2e4bc3 Compare June 20, 2026 00:49
jkmassel added 2 commits June 21, 2026 11:04
`%*d` / `%.*f` consume an extra `int` argument (the width/precision) that the
one-slot-per-specifier signature model can't represent, so the regex matched
them but undercounted — making `%d` and `%*d` compare as compatible. Concretely,
changing a base string from `%.1f km` to `%.*f km` shifts the call site from one
argument to two, yet the check would wave it through, so every unchanged
translation would render the precision where the value belongs.

Stop matching `*` in the width/precision, leaving such a specifier unrecognized:
a change to or from a `*` form now surfaces as a difference and gets flagged
rather than silently passing. Fixed width/precision/length specifiers are
unaffected.
The suite stubs `UI.abort_with_message!` to a no-op so the other examples can
inspect the return value, which meant nothing proved the abort actually
terminates the lane — it was only asserted as "received". Add one example that
lets the real abort run (`and_call_original`) and asserts the action raises
rather than falling through to its return. Verified it bites: swapping the abort
for a non-halting `UI.error` makes this example fail.
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.

2 participants