fix(title_case): don't capitalize am/pm in time expressions#3456
fix(title_case): don't capitalize am/pm in time expressions#3456jlaportebot wants to merge 8 commits into
Conversation
The title case linter was incorrectly capitalizing 'am'/'pm' in time expressions like '9:05am' to '9:05Am'. Both 'am'/'pm' and 'AM'/'PM' are valid after numbers, so the linter should leave them as-is. Add is_time_am_pm() helper that detects when 'am' or 'pm' follows a number (possibly with a colon, e.g. '9:05am' or '5pm'), and skip title-casing such words. Fixes Automattic#3454.
hippietrail
left a comment
hippietrail
left a comment
There was a problem hiding this comment.
Looks interesting. Thanks. I noticed one place where you can save a tiny type conversion.
| let chars = word.get_ch(source); | ||
| let chars_lower = chars.to_lower(); | ||
|
|
||
| if !(chars_lower.as_ref() == ['a', 'm'] || chars_lower.as_ref() == ['p', 'm']) { |
There was a problem hiding this comment.
You can avoid .to_lower() and use .eq_ch() from CharStringExt directly. I think there's also .eq_any_ascii_ignore_case() either on the same type or at least on Span too.
|
@jlaportebot, please update all of your PRs to comply with our agent policy. |
…q_any_ignore_ascii_case_str - Replace .to_lower() + manual comparison with eq_any_ignore_ascii_case_str for cleaner matching (per review feedback) - Add 'continue' to skip processing when is_time_suffix is true, preventing the else branch from lowercasing AM/PM
|
Thanks for the review @hippietrail! I've addressed both points:
|
The proper-noun dictionary lookup was running before the time-suffix check, causing 'AM' to be lowercased to 'am' even when it appeared after a number in a time expression. Moving the is_time_suffix check earlier ensures time-suffix words are skipped entirely before any case transformation can occur.
|
Updated PR body with the full AI Disclosure section from the template. Also fixed the actual test failure — the |
|
The macOS x64 Desktop check failed, but all other checks (including macOS arm64, Rust checks, JS checks, and all plugin tests) passed. The macOS x64 failure appears to be a CI infrastructure issue — the job logs are no longer available (blob not found), and the title_case change only touches |
…tr variant Follow reviewer feedback to use char-slice comparisons rather than string-slice comparisons for the am/pm check, avoiding an implicit &str → &[char] conversion.
|
Thanks @hippietrail — good catch. This has been addressed in the latest commits: the |
3 participants
Summary
The title case linter incorrectly capitalizes
am/pmin time expressions like9:05amto9:05Am. Both lowercase and uppercase variants are valid after numbers, so the linter should leave them untouched.Changes
is_time_am_pm()helper intitle_case.rsthat detects whenamorpmfollows a number (possibly with a colon, e.g.9:05amor5pm)am/pm/AM/PMas-istitle_case.rsuse_title_case.rsTest Plan
cargo check -p harper-core --libpassescargo test -p harper-core)Fixes #3454
AI Disclosure
If Your PR Implements or Enhances a Linter
Checklist