feat: add week duration support to parse_duration_secs#2324
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10afd573ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "m" => Some(n * 60), | ||
| "h" => Some(n * 3600), | ||
| "d" => Some(n * 86400), | ||
| "w" => Some(n * 604800), |
There was a problem hiding this comment.
The new "w" branch computes n * 604800 without overflow checks, so a large user-supplied since like "30000000000000w" can overflow (u64 panic in debug builds, wrap in release) and produce a bad cutoff or crash channel.read. Since this parser already returns Option, overflow should map to None via checked_mul rather than silently overflowing.
Useful? React with 👍 / 👎.
btucker
left a comment
btucker
left a comment
There was a problem hiding this comment.
Clean, minimal change. Correctness and test coverage look good.
One minor observation: parse_duration_secs uses split_at(len - 1) which assumes a single-character suffix. This works fine for w, but if someone later wants multi-char suffixes (e.g., "mo" for months), the parser would need restructuring. Not a problem for this PR — just noting for future reference.
No issues found. LGTM.
🌃 Co-built with Midtown
|
Codex P2 (overflow guard) is already addressed in this PR — all multiplications use |
|
/rebase |
Add "w" suffix to parse_duration_secs so users can specify durations like "1w" or "2w" for the channel.read `since` parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use checked_mul for all duration suffix multiplications (m, h, d, w) so that large user-supplied values return None instead of panicking in debug or wrapping in release. Updated doc comment to document the overflow behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2324 +/- ##
==========================================
+ Coverage 63.89% 63.95% +0.05%
==========================================
Files 100 100
Lines 38581 38582 +1
==========================================
+ Hits 24653 24675 +22
+ Misses 13928 13907 -21
🚀 New features to boost your workflow:
|
1 participant
Summary
"w"(week) suffix toparse_duration_secs, so users can use--since 1wor--since 2wwithchannel.read1w= 604800s,2w= 1209600s)Test plan
weekstest case asserting correct conversionparse_duration_secstests still passcargo clippycleanCloses !9
🌃 Co-built with Midtown