Localization foundations, with Brazilian Portuguese as the first translation - #154
Open
jacksonfdam wants to merge 23 commits into
Open
Localization foundations, with Brazilian Portuguese as the first translation#154jacksonfdam wants to merge 23 commits into
jacksonfdam wants to merge 23 commits into
Conversation
Address Task 4 code review findings: every entry now carries a translator-facing comment (including the generatesSymbol rationale for the two Re-checking entries), "Run Fix" now sorts before "Run diagnostics..." to restore alphabetical order, and the file is reformatted to match BrewUIComponents' Xcode pretty-printer style (space-before-colon, compact stringUnit lines). No key or value changed.
Commit 4aad734 re-serialized this file with a JSON dumper and silently reverted the Xcode-style formatting applied in eef117c. Reformat it to match Sources/BrewUIComponents/Resources/Localizable.xcstrings exactly (space before colons, two-space indentation, collapsed simple stringUnit blocks) so both catalogs stay consistent whenever Xcode resaves one of them. No key, value, or comment content changed.
The header title, the copy button title and its confirmation lived in computed properties rather than in a view initialiser, so the module sweep missed them and the Doctor pane rendered "Terminal command" and "Copy" beside Portuguese. Carrying them needs BrewActionButton and CommandBlockView's own title to take a LocalizedStringResource: resolving a resource to a String at the call site would bake in the wrong language and lose the owning module's bundle.
Caller-supplied copy is a LocalizedStringResource, per CONVENTIONS.md. Both of NoteCallout's callers render text that must not be translated — brew doctor's own preamble, and a package's caveats as Homebrew publishes them — so they take a verbatim: route named after Text(verbatim:), which makes opting out of localization something a call site has to say out loud.
LocalizedStringResource interpolates another resource directly, producing the same "Severity: %@" key. The nested String(localized:) collapsed the severity name against Locale.current while the view body was evaluating, for no reason beyond the receiving API once taking a String.
FORMULA and CASK are Homebrew's own domain terms, not app copy. CONVENTIONS.md now requires text left out of a catalogue to carry a comment saying why.
BrewUIElement is documented as the only layer that touches XCUIElement, and it resolves by descendants(matching: .any) — so the lookup no longer depends on guessing which element type SwiftUI surfaces the container as, which was the open question the removed comment recorded. It also self-waits and produces the project's standard failure diagnostics, the way .doctorScreen already does.
The rule was: every string everywhere must have Portuguese. That makes a translation a standing obligation on maintainers who never agreed to one — add an English button to a localized module and the build goes red until you produce pt-BR for it. Which languages the project accepts is the project's decision. The rule is now: a catalogue that translates any of its keys into a language translates all of them. It still catches the half-translated module the test exists for, while a catalogue nobody has started translating passes. Also fixes two flaws in the same file. The Sources/ walk ended in `?? []`, so a renamed directory would have left the suite passing having checked only the deliberately empty app catalogue; the catalogues that exist are now named and their absence throws. And the failure label for Homebrew/Localizable.xcstrings walked two directories up into the checkout folder, whose name is whatever the person cloning chose.
Both exist only so the Xcode test target can reach a bundle that is otherwise internal, and the staged plan adds one per localized module. @_spi keeps the escape hatch open for BrewTests without growing the package's public surface, which .periphery.yml's retain_public: false would then have to carry.
"pro" is a spoken contraction; product UI reads in the written register.
Resolving the lead and the relative phrase separately is correct — they belong to different catalogues — but it fixes the order and the separator, which is the classic trap for a language that wants either one different. Say so, so the current shape is not read as the general answer.
The completeness test's rule changed, component APIs now take resources, and the bundle accessors are SPI. Each was written down as something else.
MikeMcQuaid
requested changes
Sep 9, 2026
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
Member
There was a problem hiding this comment.
Hold off on this for now please. Want to get a stable release out and in homebred-cask before we introduce any localisations yet 🙇🏻
|
Ok, no worries, any other feature / issue that could be helped? |
Member
|
@jacksonfdam no, not yet. Next week! |
MikeMcQuaid
self-requested a review
September 13, 2026 14:10
Collaborator
|
@jacksonfdam : Thanks for your patience. As this is an architectural / conventions decision, I'm going to take a bit more care with this one. I've got a few more pressing issues to deal with just now, but I'll come back to this soon for sure. In the meantime, I'd appreciate if you could trim down the comments I bit. They seem possibly a bit LLM generated? I'd appreciate if we could only add comments when they're either documenting public API / shared components or explaining something genuinely strange / unconventional which the code itself doesn't explain super well. |
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
4 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.
PR: Localization foundations, with Brazilian Portuguese as the first translation
Summary
BrewUI ships English only today — no
.lproj, no String Catalogs, nodefaultLocalization. This is a proposal, not a finished feature: it adds localization infrastructure any language can plug into, and translates two modules into Brazilian Portuguese to prove the infrastructure fits real code rather than a toy example.I opened it as a PR rather than an issue because the mechanism is easier to argue about with the code in front of you. If the project would rather settle policy first, I am happy to close this and move the discussion to an issue — see Questions for maintainers below.
Coverage is deliberately partial, and the PR says so plainly:
BrewUIComponentsandBrewFeatureDoctorare translated. Installed, Upgrades, Discover, Console, Config and the app shell are not, and error copy is English everywhere. A user selecting Portuguese today gets the Doctor tab in Portuguese and the rest in English.Changes
Infrastructure
defaultLocalization: "en"inPackage.swift; each localized target declaresResources/Localizable.xcstringsas a processed resource.LocalizedStringResourcerather thanString, so they resolve at display time against the bundle that owns them.LastUpdatedLabel(lead:)is the case that forces this: the lead phrase belongs to the calling module's catalogue.LocalizedStringResource.init(<module>:)supplyingBundle.module.LocalizedStringResource(_:)andString(localized:)default toBundle.main, which in a SwiftPM module is the app — and resolving against the wrong bundle does not throw, it returns the key, so the string silently stays English.knownRegionsgainspt-BR;Homebrew/{en,pt-BR}.lproj/InfoPlist.stringsmake the app bundle itself advertise both languages, which is what macOS reads for the per-app entry in System Settings › General › Language & Region.Translation
BrewUIComponents(13 keys) andBrewFeatureDoctor(24 keys), bothen+pt-BR, every entry carrying a translator-facingcomment.RelativeTimeTextno longer builds"1 minute ago"/"5 minutes ago"with acount == 1ternary. The count is interpolated so the catalogue key is%lld minutes agoand per-language plural variations decide the form — plural rules differ by language, so a Swift-side ternary can only ever be right for one.Not translated, on purpose
DoctorCopy.warningPreamble— a verbatim echo ofbrew doctor's own output.NoteCalloutgained an explicitverbatim:route so this cannot be mistaken for localizable copy.FORMULA/CASKbadges, SF Symbol names,AXIDvalues. Each carries a comment saying why.Tests
StringCatalogueCompletenessTestsreads every.xcstringsas JSON and fails if a catalogue is internally inconsistent — if it declares a language for one key it must declare it for every key. A catalogue that has not been translated at all passes. See Questions for maintainers.Tests/suites assert catalogue keys and bundle bindings;BrewTests/LocalizationResolutionTestsasserts resolved prose. That split is forced by the toolchain — see Why this split.BrewUITests/Screens/DoctorScreen.swiftmatched the healthy state by its English copy, which breaks the moment the app runs in another language. It now resolves throughBrewUIElementandAXID.doctorHealthyState, whichCONVENTIONS.mdalready asked for ("never write a raw identifier string in a view or a test").Docs — a
Localizationsection inCONVENTIONS.md, a dated entry in.ai/memory.md, and the catalogue location inARCHITECTURE.md.Why this split
Two modules, not all seven.
BrewFeatureDoctorcannot be localized alone: it renders copy owned byBrewUIComponents(the command block header, Retry, the relative timestamp), so translating only the feature would leave a half-translated screen. Those two are the smallest coherent unit. Every other module is a mechanical repeat of the pattern this PR establishes.A toolchain fact worth knowing before reviewing the tests.
swift buildcopiesLocalizable.xcstringsinto the module bundle raw — SwiftPM's native builder runs no Apple resource compiler at all, which is also whyMedia.xcassetsarrives uncompiled and whyBrewColorTokenContrastTestsalready parses that JSON from source rather than resolvingNSColor(named:).xcodebuilddoes compile catalogues, into<lang>.lproj/Localizable.strings. CI runs both legs, andBrew-Unitcontains only theBrewTeststarget, so anything underTests/runs exclusively underswift test. A resolved-translation assertion placed there passes locally under Xcode and fails in CI. Hence: keys and bundle bindings underTests/, resolved prose inBrewTests/.One Xcode quirk.
"Re-checking"(a VoiceOver label) and"Re-checking…"(the header subtitle) derive the same identifier in theGenerateStringSymbolsphase, which strips punctuation — a hard build failure, not a warning. Both entries carry"generatesSymbol": false, safe because nothing references the generated symbols.Testing
scripts/test— 961 tests. Two failures, both pre-existing on untouchedmain:DiscoverListRowViewModelTestsandDiscoverPackageDetailViewModelTestsassert"12,345"and get"12 345", becauseinstalls30DayLabelformats with the system locale and my region uses a space as the thousands separator. Unrelated to this PR — I can send a two-line fix separately if useful.xcodebuild test -scheme Brew-Unit— TEST SUCCEEDED, including the four resolution tests that provept-BRreally comes back from the compiled catalogues.mint run swiftformat --lint .— 0 of 374 files need formatting.mint run swiftlint lint --strict— 0 violations, 349 files.BrewUILintoverHomebrew+Sources— clean.en.lproj/pt-BR.lproj, and compiledLocalizable.stringsin both embedded module bundles containing the Portuguese values.-AppleLanguages '(pt-BR)'and confirmed the Doctor surface renders in Portuguese, with thebrew doctorpreamble still in English.scripts/test-uinot run. My shell lacks the Accessibility/Automation/Screen Recording permissions thatBrewUITests/TROUBLESHOOTING.mditem 1 describes, so the runner never bootstraps — an unrelated, untouched UI test fails identically. TheDoctorScreenchange compiles and uses the same resolution path as identifiers that pass in CI today, but it has not been exercised at runtime. Please treat CI'sui-testjob as the real check on it.PR checklist
The code, the catalogues and this description were written with Claude Code, working from
AGENTS.mdandCONVENTIONS.md, and reviewed by me before each commit. Verification was not left to the model: every command in the Testing section above was actually run and its output read, the two pre-existing failures were confirmed against untouchedmainby checking it out with a clean tree, the Portuguese translations were checked by a native speaker (me), and theBundle.moduleand.lprojclaims were verified by inspecting the built app bundle rather than inferred. The two limits are stated plainly rather than papered over: the UI test is unverified at runtime, and coverage is two modules.Questions for maintainers
Does the project want translations at all, and on what terms? There is no prior localization issue here, so this PR is also the question. Who owns a translation once it lands, what happens when one goes stale, and which languages are accepted? I did not want to assume an answer, which is why the completeness test checks consistency within a catalogue rather than demanding Portuguese for every new string — nothing here obliges a maintainer to produce a translation to merge an English string.
Should the app advertise
pt-BRbefore coverage is meaningful? As it stands, System Settings will offer Portuguese while roughly a tenth of the app is translated. The alternative is to dropHomebrew/{en,pt-BR}.lproj/InfoPlist.stringsfrom this PR and add them once coverage justifies it — the translations still ship, they are just not selectable yet. Happy to do that; it is two files.Is the staging shape right? I would rather send the remaining modules as separate PRs than grow this one.
Follow-ups
BrewFeatureInstalled,BrewFeatureDiscover,BrewFeatureConsole,BrewFeatureConfig, and the app shell.LoadState's failure payload isString, produced byOperationFailure.userFacingMessageinBrewCoreand consumed by all five feature modules — migrating it touches all of them at once and belongs in its own PR.localized:call sites pass nobundle:(BrewFeatureInstalled46,BrewFeatureDiscover27,BrewFeatureConfig3,BrewServicesTestSupport2,BrewCore2,BrewRepositories1). Harmless while those targets have no catalogue — the call returns its English key, which is today's behaviour — but each is wrong in a way that only becomes visible when its module is translated. A follow-up should fix them together with a BrewUILint rule making the argument mandatory, since the rule fails the tree until every site is fixed.Stringfor caller-supplied copy (CommandBlockView.summaryText,PackageDetailSectionHeading.title,ErrorStateView.message); each is fed by a view model in a module this PR does not touch.