fix(hig): drop the em dash from five on-screen strings - #2199
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Follow-up to #2194, which reported this while working nearby.
CLAUDE.mdbans the em dash from anything a user reads. Five on-screen strings still had one:JSON — <column>JSON: <column>PHP — <column>PHP: <column>Sync paused — Pro license expiredSync paused, Pro license expiredNULL — no referenced rowNULL, no referenced row— <warning>, <warning>Why the comma, and why the colon
The comma is the house style, not a guess. The repo has already migrated four identical em dashes and every one became a comma:
Truncated, read only,License expired, sync paused, and both~/.pgpass found, ...strings.License expired, sync pausedmatters most here, because it is the live sibling of the Account banner. A colon there would punctuate the same fact two different ways on two adjacent surfaces.The colon stays for the two window titles, where the left side is a bare label rather than a clause.
The import list is the one place a colon would have been actively wrong. The subtitle immediately before it is
\(host):\(port), rendered in anHStack(spacing: 0)underlineLimit(1), so a colon would producelocalhost:5432: Some values are set outside this file: two colons, two meanings, one line.The JSON title was also unlocalized
It was built as
columnName.map { "JSON — \($0)" }, raw interpolation with noString(localized:), so the key never reached the catalog and the title was English in every locale. It now usesString(format: String(localized: "JSON: %@"), columnName), the shapeCLAUDE.mdrequires and the one the PHP twin already used, so it is translatable for the first time.What was deliberately left alone
JetBrainsCredentialStore.swift:44,48,52. Three Keychain service names embed an em dash because they mirror IntelliJ's owngenerateServiceNameformat,IntelliJ Platform <subsystem> — <key>. They are lookup keys, not prose, and rewriting them would break JetBrains credential import. They spell the character as\u{2014}, so a literal-character grep does not surface them, which is exactly how a blanket sweep would have broken this.Text(verbatim: "—"),?? "—") in the integrations panes and the metadata models. A bare em dash meaning "empty" is the macOS convention Finder and Activity Monitor use. It is a glyph, not a sentence to rewrite.CHANGELOGsections, andappcast.xml. Not user-facing, or a frozen record of what already shipped.[Unreleased]had none, anddocs/is already clean.Translations
The catalog is compiler-extracted, and a command-line
xcodebuilddoes not write back to it: I measured the file hash before and after a full build and it did not move. So the four renamed keys keep their old translations and the new keys are absent, which means these five strings fall back to their English source intr,vi,zh-Hansandzh-Hantuntil the catalog is regenerated in Xcode and refilled.No em dash reaches any user either way, because the fallback is the fixed English. This also matches what the repo already does:
decd6952erenamed strings the same way and627a03a34refilled the translations a month later.Five values are worth carrying over when that happens. The rest of the old values are verbatim English copies with nothing to preserve.
NULL — no referenced rowNULL, referans satır yokNULL — no referenced rowNULL, không có dòng tham chiếuNULL — no referenced rowNULL,无引用行Sync paused — Pro license expiredTạm dừng đồng bộ, giấy phép Pro đã hết hạnSync paused — Pro license expired同步已暂停 - Pro 许可证已过期(already has no em dash)zh-Hantalready readsNULL,沒有參照的列and同步已暫停,Pro 授權已過期, so both are correct as they stand.I did not hand-edit
Localizable.xcstrings: the repo hook warns it is shared and frequently dirty from other work, and hand-writing 2.4 MB of catalog to save five values is the riskier trade.The guard
UserFacingEmDashGuardTestsreads the app sources and fails on an em dash inside a string literal. It exempts the placeholder glyph, comments and log messages, and needs no exemption for the JetBrains names because those spell the character as an escape.A guard test that cannot fail is worse than none, so I checked this one bites: reintroducing
Text("NULL — no referenced row")fails it. A companion suite tests the classifier directly on each case it has to get right, including the escaped spelling and the multi-line log call.Verification
generate: PASSbuild(Debug): PASStestUserFacingEmDashGuardTests, UserFacingEmDashClassifierTests: PASS, 6 executed, 6 passedswiftlintoverTableProandTableProTests: 0 violationsNo UI automation: these are static strings with no flow to drive, and the source-scanning guard covers them more directly than a UI test could.