Improve .strings handling: unquoted strings, inline comments, and merge prefixing

[jkmassel]

Summary

Related improvements to the existing .strings handling — the duplicate-key scanner (StringsFileValidationHelper) and the merge_strings helper — extracted from #738 so the new ios_lint_localization_placeholder_changes action, which builds on these, can be reviewed on its own. This PR stands alone and is independently useful.

The common thread: each change accepts more of the unquoted-string grammar plutil itself allows, so legitimate InfoPlist.strings-style input — unquoted keys/values, inter-token comments — is handled instead of rejected.

Changes

1. Parse unquoted .strings values and widen the unquoted grammar

StringsFileValidationHelper already tokenized unquoted keys; it now also tokenizes unquoted values and accepts the full character set plutil allows for unquoted strings (alphanumerics plus _ . - $ : /). So an InfoPlist.strings-style file like CFBundleName = WordPress; parses instead of raising Invalid character. The scanner only ever runs on input plutil has already accepted, so matching its grammar keeps a parseable file from tripping it; a genuinely un-tokenizable plist (e.g. a nested-dictionary value) still fails closed.

2. Accept comments placed between a statement's tokens

StringsFileValidationHelper recognized comments only in its top-level :root context, so a comment between a statement's tokens — "key" /* note */ = "value";, "key" = /* note */ "value";, or "key" = "value" /* note */; — raised Invalid character, even though plutil accepts all three. The scanner now threads a resume_context so a comment returns to the state it interrupted instead of always dropping back to :root. The lone ambiguous spot — a / right after =, which could begin a comment or a /usr/bin-style unquoted value — is disambiguated one character later via a new :maybe_comment_or_value state, so /-leading unquoted values still parse. ✅

3. Centralize duplicate-key detection behind scan_for_duplicate_keys

ios_lint_localizations open-coded a strings_file_type == :text ? scan : warn gate. That logic now lives in StringsFileValidationHelper.scan_for_duplicate_keys, which detects the file format and returns a [:scanned | :unsupported_format | :unscannable, payload] tri-state; each caller maps it to its own policy. As a side effect, ios_lint_localizations' check_duplicate_keys no longer crashes the lane on a valid-but-not-flat-.strings file — it warns via UI.important and skips it.

4. assume_valid: to skip a redundant plutil -lint

L10nHelper.strings_file_type gains an opt-in assume_valid: flag that skips the plutil -lint validity check when the caller has already parsed the file. Default behavior is unchanged for every existing caller. (Used by #738.)

5. Apply the merge_strings key prefix to unquoted keys and values

L10nHelper.merge_strings rewrites each key to add the caller's prefix, but its line matcher only handled [A-Z0-9_] keys with a quoted value. An unquoted key containing . - $ : / (e.g. a reverse-DNS key), or any line with an unquoted value (CFBundleName = WordPress;), was written to the merged file without the prefix — while still being bookkept with it. That inconsistency can resurface the very collisions the prefix exists to avoid and break downstream key extraction. The matcher now accepts the full unquoted-string set plutil allows; the unquoted-value case keys on the fact that an unquoted string can't contain spaces, so it won't mistake comment prose like and = a red herring … for a key/value pair (a case the fixtures deliberately guard). ✅

Test Plan

• bundle exec rubocop — no violations.
• bundle exec rspec — 914 examples, 0 failures; new coverage for unquoted values, the widened key set, scan_for_duplicate_keys, the graceful warn-and-skip, assume_valid, inter-token comments, and merge_strings unquoted-key/value prefixing.
• CHANGELOG updated.
• MIGRATION.md — N/A (additive + backward-compatible).

Related

• Add ios_lint_localization_placeholder_changes action #738 will be rebased to depend on this.

[mokagio]

Nice appraoch.

[mokagio]

My AI noticed that merge_strings doesn't handle inter-token comments.

Could this logic handle the same inter-token comment placements that StringsFileValidationHelper now accepts? (https://github.com/wordpress-mobile/release-toolkit/pull/741/changes#diff-6e464dfe8ccd3974026649f8aaa02f9f8518261ef391f13bdb9535ef46d2b908R111-R116)

For example, CFBundleName /* c */ = WordPress; is accepted by plutil and by the duplicate-key scanner after this PR, but merge_strings still writes it unprefixed while bookkeeping it as prefixed.
That can reintroduce collisions in the merged file.

I added failing specs in #742 to pin the gap.

AI suggests: A tokenizer-backed rewrite would be ideal, but a comment-aware interim rewrite would also close the regression.

[jkmassel]

@mokagio – I merged #742 then applied 0957623, which fixed the underlying issue

[mokagio]

Nicely done @jkmassel ! I got my agent to poke around more and it found this #750

To be honest, I can't tell whether that's a legit concern for our use case because "k" = { a = b; };, while a valid plist apparently, is not in a shape I've ever seen working in our domain. So, I guess it's a matter of deciding whether to investigate how to improve the logic in the abstract, or to stay focused on the practical use case we have.

If you need this addressed ASAP, then by all means let's merge and ship. But if you have the time to investigate further, then maybe it'd be nice to address?