Skip to content

fix: support HH:MM am/pm time formats in combined date-time parsing - #284

Merged
sylvestre merged 2 commits into
uutils:mainfrom
0xSoftBoi:fix/hhmm-ampm-parsing
Jul 5, 2026
Merged

fix: support HH:MM am/pm time formats in combined date-time parsing#284
sylvestre merged 2 commits into
uutils:mainfrom
0xSoftBoi:fix/hhmm-ampm-parsing

Conversation

@0xSoftBoi

Copy link
Copy Markdown
Contributor

Summary

  • The combined date-time parser (combined::parse) used time::iso which only accepts 24-hour notation, causing inputs like "2024-06-15 12:00 PM" to fail — the ISO parser greedily consumed "12:00" and left "pm" orphaned
  • Switched to time::parse (which tries am_pm_time first with fallback to iso), so am/pm suffixes are now correctly consumed as part of the time
  • Case insensitivity is already handled by parse_items lowercasing input before parsing

Fixes#282

Test plan

  • Added test date_and_time_ampm covering all three failing inputs from the issue: "2024-06-15 12:00 PM", "2024-06-15 11:30am", "2024-06-15 3:00 PM"
  • All 376 existing tests pass (cargo test)
  • No regressions — am_pm_time fails gracefully (backtrack) on inputs without meridiem, falling through to iso

@codspeed-hq

codspeed-hqBot commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 3.84%

❌ 1 regressed benchmark
✅ 20 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

BenchmarkBASEHEADEfficiency
parse_extended_year_relative31.4 µs32.6 µs-3.84%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing 0xSoftBoi:fix/hhmm-ampm-parsing (8c06463) with main (4b412e0)

Open in CodSpeed

@codecov

codecovBot commented Apr 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.31%. Comparing base (4b412e0) to head (8c06463).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #284 +/- ##
=======================================
Coverage 99.30% 99.31% =======================================
Files 20 20 Lines 3894 3941 +47 Branches 122 127 +5 =======================================
+ Hits 3867 3914 +47 
Misses 26 26 Partials 1 1 
FlagCoverage Δ
macos_latest99.31% <100.00%> (+<0.01%)⬆️
ubuntu_latest99.31% <100.00%> (+<0.01%)⬆️
windows_latest14.26% <34.69%> (+0.21%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sylvestre

Copy link
Copy Markdown
Contributor

i am surprised to see potential perf regression here. what is your take here?

@tomagsx

Copy link
Copy Markdown

Review bump on this one. Functional coverage looks good and CI is green aside from the CodSpeed regression flag. The remaining question seems to be whether the measured perf hit is acceptable for the am/pm fix, so I’d really appreciate a maintainer take on that tradeoff when you have a chance.

@0xSoftBoi

Copy link
Copy Markdown
ContributorAuthor

@sylvestre good catch — wanted to actually look into it rather than just acknowledge it on CodSpeed.

The two regressed benchmarks aren't on a code path this PR touches:

  • parse_weekday (benches/parse_datetime.rs:57-58) calls parse_datetime("wednesday") — pure weekday parsing, never enters items::combined.
  • parse_extended_year_relative (benches/parse_datetime.rs:109-115) calls parse_datetime_at_date(..., "10000-01-01 +1 day") — that uses the extended-year branch, also outside items::combined.

The PR's only modified file is src/items/combined.rs. Commit fdef5ec keeps the existing ISO fast path intact: it tries time::iso first, only falls back to time::parse (the more permissive parser that handles am/pm) if the remaining input begins with am/pm/a.m./p.m.. So well-formed ISO inputs without meridiem still take exactly the same path as before.

The ~4% deltas on those two unrelated benchmarks look like CodSpeed measurement variance — similar small regressions on the same parse_weekday benchmark show up on my other open PRs against this branch (#285, #286) which touch entirely different files.

Happy to acknowledge it on CodSpeed, or if you'd prefer I can also add a microbench specifically for the combined-datetime fast path so we have a more direct signal next time.

0xSoftBoiand others added 2 commits June 7, 2026 14:17
The combined date-time parser (e.g., "2024-06-15 12:00 PM") used
time::iso which only accepts 24-hour notation. When an am/pm suffix
followed, iso would greedily consume the time portion, leaving the
meridiem orphaned and causing a parse error.
Switch to time::parse which tries am_pm_time first (with fallback to
iso), so inputs like "2024-06-15 12:00 PM", "2024-06-15 11:30am", and
"2024-06-15 3:00 PM" are now accepted. Case insensitivity is already
handled by parse_items lowercasing the input before parsing.
Fixesuutils#282
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@0xSoftBoi
0xSoftBoiforce-pushed the fix/hhmm-ampm-parsing branch from fdef5ec to 8c06463CompareJune 7, 2026 18:19
@sylvestre
sylvestre merged commit 5255467 into uutils:mainJul 5, 2026
20 of 21 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

date input: HH:MM am/pm forms rejected (e.g. 2024-06-15 12:00 PM)

3 participants

@0xSoftBoi@sylvestre@tomagsx