Add failing specs for inter-token comments in merge_strings#742
Merged
Conversation
These specs fail against the current `merge_strings`, pinning a gap left by PR #741: the duplicate-key scanner and `plutil`-based bookkeeping both accept `.strings` comments *between* a statement's tokens (e.g. `CFBundleName /* c */ = WordPress;`), but the line-based key rewrite only prefixes when `=` and the value are adjacent. A key behind such a comment is written unprefixed while bookkept *with* the prefix — resurfacing the very key collisions the prefix exists to prevent, as the second spec demonstrates by merging two files into a single collapsed key. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Generated by 🚫 Danger |
Assert parsed keys so the regression captures the merge contract without depending on raw output formatting. --- Generated with the help of Codex, https://openai.com/codex Co-Authored-By: Codex GPT-5 <noreply@openai.com>
4 tasks
jkmassel
added a commit
that referenced
this pull request
Jun 30, 2026
`merge_strings` prefixed keys with line-based regexes that assume the key, `=`, and value are adjacent, so an inter-token `.strings` comment — `AppName /* c */ = WordPress;` or `CFBundleName = WordPress /* trailing */;` — broke the match and the key was written to the merged file without the prefix, while the `plutil`-derived bookkeeping still recorded it *with* the prefix. The inconsistency could resurface the very collisions the prefix exists to avoid (two files sharing a key behind a comment merged to a single colliding key). Replace the regexes with `StringsFileValidationHelper.prefix_keys`, a comment-aware tokenizer that reuses the same state machine `find_duplicated_keys` uses: it prefixes a key wherever the tokenizer enters one, regardless of surrounding comments, and leaves `key = value`-looking text inside a comment alone. Bookkeeping and rewriting now agree by construction. Fixes the failing specs added in #742.
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
3 participants
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 two failing specs that pin a gap in #741, on which this branch is stacked.
AI-generated details
The duplicate-key scanner and
plutil-based bookkeeping both accept.stringscomments between a statement's tokens (e.g.CFBundleName /* c */ = WordPress;), butmerge_strings' line-based key rewrite only prefixes when=and the value are adjacent. A key behind such a comment is written unprefixed while still bookkept with the prefix — resurfacing the key collisions the prefix exists to prevent. The second spec makes the harm concrete: merging two files that each hide the same key behind a comment collapses to a single key instead of two distinctly-prefixed ones.The specs are red on purpose — they're the regression net for the fix (tokenizer-unification, or an interim comment-aware rewrite), which is intentionally not in this branch yet.
Heads-up for reviewers
foundation, nottrunk— review on top of Improve.stringshandling: unquoted strings, inline comments, and merge prefixing #741.nokogiri1.19.3 from source (the repo forces the ruby platform); CI's macOS agents build it fine. Drop in the precompilednokogiri-...-arm64-darwingem if running locally.How to test
bundle exec rspec spec/ios_l10n_helper_spec.rb— the two new#merge_stringsexamples fail; the rest pass.