llm: Capture review learnings on alphabetization, comments, localization, and what-not-to-test#2612
llm: Capture review learnings on alphabetization, comments, localization, and what-not-to-test#2612SaintPatrck wants to merge 2 commits into
Conversation
…ion, and what-not-to-test
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Documentation-only PR adding guidance to three iOS skill files: alphabetization rules, comment intent, localization key conventions, tightened TODO-ticket guidance, and a "don't test Swift-synthesized conformances" gotcha. Cross-references between the checklist, style-patterns, and testing-gotchas files line up. Statements about Swift's Code Review DetailsNo findings. |
| Source: [contributing.bitwarden.com — Swift style: Alphabetization](https://contributing.bitwarden.com/contributing/code-style/swift#alphabetization). | ||
|
|
||
| - [ ] Enum cases are alphabetized — unless the raw type encodes ordering (e.g., `Int` raw values that line up with server error codes or persisted indices). Document the carve-out inline if you take it. | ||
| - [ ] Tests within a test file are alphabetized by function name. |
There was a problem hiding this comment.
🤔 Except in cases where they're not—in particular, within a group of tests for a function, we'll sometimes put the error-case tests at the end.
| ```swift | ||
| @Test | ||
| func rawValues_matchServerContract() { | ||
| #expect(BankAccountType.checking.rawValue == "checking") |
There was a problem hiding this comment.
🤔 Is there a way we could make this generic instead of referencing explicit bank account types?
|
|
||
| Source: [contributing.bitwarden.com — Swift style: Localization](https://contributing.bitwarden.com/contributing/code-style/swift#localization). | ||
|
|
||
| - [ ] Localization keys mirror the English string closely. `BankAccount` for "Bank account", not `TypeBankAccount` or `BankAccountTitle`. |
There was a problem hiding this comment.
🤔 Could we make this generic instead of referencing bank accounts?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2612 +/- ##
==========================================
- Coverage 87.21% 86.21% -1.00%
==========================================
Files 1898 2125 +227
Lines 168089 183829 +15740
==========================================
+ Hits 146599 158491 +11892
- Misses 21490 25338 +3848 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…c examples - Note that error-case tests within a per-function group are commonly placed at the end rather than strictly alphabetized. - Replace BankAccount-specific examples with generic Status/Archive stand-ins so the docs don't go stale when the feature ships.
| ## Localization | ||
|
|
||
| Source: [contributing.bitwarden.com — Swift style: Localization](https://contributing.bitwarden.com/contributing/code-style/swift#localization). | ||
|
|
||
| - [ ] Localization keys mirror the English string closely. `BankAccount` for "Bank account", not `TypeBankAccount` or `BankAccountTitle`. | ||
| - [ ] Translator-facing comments (`/* … */` above a key) describe meaning, placement, or constraints that affect translation — not internal phasing or ticket bookkeeping. Translators are the audience. |
There was a problem hiding this comment.
🎨 We use DescriptionLong suffix for string keys that would be really long if they'd mirror the English string value. I would say more than 70 or 80 characters would need this suffix.
🎟️ Tracking
No ticket — captures review learnings from #2573.
📔 Objective
Roll learnings from PR #2573 review into the project's iOS skill set so the same feedback doesn't have to be given on every new feature PR.
Style guidance (
.claude/skills/reviewing-changes/reference/ios-style-patterns.md):Testing guidance (
.claude/skills/testing-ios-code/references/testing-gotchas.md):Codable/CaseIterablefor plain raw-value enums. Includes a workedBankAccountType-style example and explicit carve-outs for non-synthesized cases.Feature-review checklist (
.claude/skills/reviewing-changes/checklists/feature-addition.md):