weekday: ignore the weekday when an explicit date is given - #335
Conversation
Merging this PR will improve performance by 3.2%
Performance Changes
Tip Curious why performance improved? Comment Comparing 
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 97.48% 97.58% +0.09%
==========================================
Files 21 21
Lines 4258 4389 +131
Branches 136 138 +2
==========================================
+ Hits 4151 4283 +132
+ Misses 106 105 -1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
When the input carries both a weekday and a calendar date, the date is now authoritative and the weekday is dropped, matching GNU date. This also covers ordinal forms such as "next friday sep 25 2026". Relative items are still applied on top of the date, and a weekday without a date behaves as before. Closes uutils#317
63edbf2 to
b0c0424
Compare
|
Quick update: I looked at GNU's source before writing the first version, and I shouldn't have. I've thrown that version out and rewritten it from scratch. This time I only ran GNU date and checked what it does. The new version is force-pushed. |
| /// - e. Apply relative adjustments (e.g., "+3 days", "-2 months"). | ||
| pub(super) fn build(self) -> Result<ParsedDateTime, error::Error> { | ||
| pub(super) fn build(mut self) -> Result<ParsedDateTime, error::Error> { | ||
| // An explicit calendar date wins over a weekday, even a mismatching |
There was a problem hiding this comment.
the doc comment just above already says this, could we drop this one or keep it to one line?
There was a problem hiding this comment.
Agreed, removed it. The doc comment already covers the weekday case and the if is clear enough without it.
| // $ TZ=UTC date -d 'wed 10:30' # next wednesday at 10:30 | ||
| // | ||
| // `date --debug` warns that the day is ignored when explicit dates are | ||
| // given. Verified against GNU coreutils 9.7, with the base below being |
There was a problem hiding this comment.
the PR description says GNU 8.32, here 9.7, which one is it?
and the latest is 9.12
There was a problem hiding this comment.
I re-ran all 22 cases against 9.12 and they match 9.7 exactly. The test comment now only mentions 9.12. The 8.32 was in the old PR description, which I've since replaced. No distro packages 9.12 yet, so I built it from the GNU release tarball.
| ); | ||
| } | ||
|
|
||
| // A weekday is ignored when an explicit calendar date is given, whether or |
There was a problem hiding this comment.
please make the comment shorter, the case names already explain most of it
There was a problem hiding this comment.
Done, it's two lines now. It just notes that the weekday is ignored when an explicit date is given, matching GNU date 9.12.
|
Thanks for your PR |
If you gave the parser a weekday and a date that didn't match, it moved the date to fit the weekday. August 17 2026 is a Monday, so
Wednesday August 17 2026should give Aug 17, but it gave Wed Aug 19.Now the date wins and the weekday is ignored, which is what GNU date does.
Wednesday August 17 2026gives Mon Aug 17.This works for:
next,lastand ordinal weekdays.next Friday Sep 25 2026now gives Sep 25, which also fixes the case in coreutils #14681.These still work the same:
+1 dayornext weekare applied after the date.wed,wed 10:30) behaves as before.The fix is one change in the builder: when a date is set, it clears the weekday. Because of that, the normal-year and large-year paths now behave the same.
I added a test with 22 cases, all checked against GNU date. 18 of them fail without the fix. One existing unit test expected the old behaviour, so I updated it to match what GNU gives.
Closes #317