Add ios_lint_localization_placeholder_changes action#738
Draft
jkmassel wants to merge 1 commit into
Draft
Conversation
Collaborator
Generated by 🚫 Danger |
jkmassel
force-pushed
the
jkmassel/issue-736
branch
2 times, most recently
from
cbbfbfd to
d2e4bc3
Compare
jkmassel
force-pushed
the
jkmassel/issue-736
branch
from
e9e9c31 to
bb81e99
Compare
4 tasks
A temporal localization guardrail: compares two versions of a base-language `.strings` file and fails when an existing key's value changes its format placeholders (count, position, or argument type), which silently breaks every translation filed under that key. - `StringPlaceholdersHelper` — a pure-Ruby, platform-agnostic primitive that reduces a value's printf/`NSString` specifiers to a canonical signature. - `ios_lint_localization_placeholder_changes` — the iOS action wrapping it, with fail-closed guards for duplicate keys and mixed positional/non-positional specifiers. Builds on the `.strings` parsing and `scan_for_duplicate_keys` helper from #741.
jkmassel
force-pushed
the
jkmassel/issue-736
branch
from
bb81e99 to
cacc5ac
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new guardrail to prevent translation-breaking changes by detecting incompatible format-placeholder changes for existing localization keys between two versions of an iOS base .strings file, backed by a reusable, platform-agnostic placeholder-signature primitive.
Changes:
- Introduces
Fastlane::Helper::StringPlaceholdersHelperto compute and compare canonical placeholder “signatures”. - Adds
ios_lint_localization_placeholder_changesFastlane action to compare old vs new base.stringsand abort on incompatible placeholder changes (and on mixed positional/non-positional placeholders). - Adds comprehensive RSpec coverage for the primitive and the new action, and documents the feature in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/string_placeholders_helper_spec.rb | Unit tests covering placeholder signature extraction, compatibility, and edge cases. |
| spec/ios_lint_localization_placeholder_changes_spec.rb | Action-level tests validating parsing, duplicate-key behavior, abort/report behavior, and failure modes. |
| lib/fastlane/plugin/wpmreleasetoolkit/helper/string_placeholders_helper.rb | New pure-Ruby placeholder signature/compatibility primitive. |
| lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_lint_localization_placeholder_changes.rb | New iOS action that parses two .strings versions and reports/aborts on incompatible placeholder changes. |
| CHANGELOG.md | Adds a Trunk entry describing the new primitive and action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
+92
| def self.placeholder_signature(value) | ||
| # Collapse the specifiers into argument *slots* keyed by position: an explicit `%1$…` keys by | ||
| # its number, a non-positional `%…` by its order of appearance. Keying (rather than listing) | ||
| # means a positional argument referenced more than once (`%1$@ … %1$@`) collapses to a single | ||
| # slot — it still consumes one argument, so it stays compatible with a single `%1$@`. Sorting | ||
| # by position keeps the signature invariant to reordering equivalent positional arguments. | ||
| slots = {} | ||
| extract_specifiers(value).each_with_index do |specifier, index| | ||
| slots[specifier[:position] || (index + 1)] = specifier[:class] | ||
| end | ||
| slots.sort.map { |position, klass| "#{position}:#{klass}" }.join(',') | ||
| end |
Comment on lines
+108
to
+114
| case status | ||
| when :scanned then payload | ||
| # A non-`:text` plist (xml/binary) has no tokenizer here, but `plutil` collapses any duplicate to | ||
| # its last value when parsing it, and toolkit-generated XML is built from a Hash and is dup-free | ||
| # anyway — so there's nothing to catch. | ||
| when :unsupported_format then {} | ||
| # A `:text` file `plutil` parses but our scanner can't tokenize (e.g. a nested dictionary/array |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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::StringPlaceholdersHelperPure Ruby, no file I/O, platform-agnostic (the
%@/%1$dplaceholder 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)— the keys present in both whose signature differs. New/removed keys are ignored on purpose: copy that needs a fresh translation is expected to land under a new key.mixed_operators?(value)— whether a value invalidly mixes positional (%1$@) and non-positional (%@) specifiers.The signature is invariant to benign copy edits and to reordering equivalent positional args, but sensitive to count/position/type changes.
%d↔%iare compatible;%%is skipped; the,/'grouping flags are recognized;*dynamic width/precision and the write-back%nare deliberately left unrecognized. A single literal%followed by whitespace is treated as a literal percent, not a specifier, so ordinary copy like"50% off"doesn't fabricate a phantom placeholder.2. iOS action —
ios_lint_localization_placeholder_changesTakes
old_file/new_file(the previous and newly-generated base.strings), parses them via the existingL10nHelper.read_strings_file_as_hash(plutil), runs the primitive, and reports. Mirrorsios_lint_localizations'sabort_on_violations(defaulttrue).It fails closed on any input where the comparison would be unreliable:
check_duplicate_keys(defaulttrue) — aborts when either file defines a key more than once (plutilsilently keeps only the last value, which could otherwise hide a real placeholder change). Delegates to the sharedStringsFileValidationHelper.scan_for_duplicate_keysfrom Improve.stringshandling: unquoted strings, inline comments, and merge prefixing #741.abort_on_violations) when a value mixes positional and non-positional placeholders — that's invalid and leaves the signature undefined.Worked examples (match the issue spec — verified by tests)
Just textHello %@1:object%1$d items in %2$@1:int,2:object%2$@ told by %1$@1:object,2:object100%% sure about %@1:object%d likes1:intNotes / scope
.stringshandling: unquoted strings, inline comments, and merge prefixing #741, which carries the.stringsparsing (unquoted keys/values, inter-token comments) and thescan_for_duplicate_keyshelper this action calls.android_*parse-and-call wrapper.L10nHelper.read_strings_file_as_hashrather than adding a second.stringsparser — the action is therefore macOS-only by design, likeios_lint_localizations..stringsdict/ ICU plurals are out of scope (likely v2).MIGRATION.mdentry — purely additive.Test Plan
bundle exec rubocop— no violations.bundle exec rspec— all green. Helper specs cover every worked example + edge cases (%%, a literal%in copy, nil/whitespace, non-ASCII, escapes, width/precision/length modifiers,%d↔%i, the,/'grouping flags,*dynamic width/precision,%n,%.f, and the invalid positional/non-positional mix). Action specs write real.stringsfixtures parsed byplutil: no-op, abort,abort_on_violations: false, add/remove ignored, duplicate-key abort, fail-closed on an un-tokenizable file, mixed-operator abort, and missing file.Related issues
.stringsparsing +scan_for_duplicate_keys) extracted to Improve.stringshandling: unquoted strings, inline comments, and merge prefixing #741.fastlane/helpers/string_placeholders.rb+ thevalidate_string_placeholderslane). WPiOS will adopt the toolkit version and delete the project-side copy once this lands.