-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use Durations for calculating relative times #85
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
===========================================
+ Coverage 70.00% 80.26% +10.26%
===========================================
Files 6 6
Lines 820 826 +6
Branches 190 182 -8
===========================================
+ Hits 574 663 +89
- Misses 127 128 +1
+ Partials 119 35 -84
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good.
Could you please review this pull request? @cakebaker
@cakebaker Could you take a look at this if you have time? |
Good work, thanks for your PR! |
This PR closes uutils/coreutils#6629.
There were two problems with
parse_relative_time
, which have been fixed by this PR.One problem was that
parse_relative_time()
got the current date usingUtc::now().date_naive()
whileparse_relative_time_at_date()
got the current date usingLocal::now().date_naive()
. This means that the date in the end result will be 1 day off if the date in your timezone is different from the date in UTC.Another problem was that
parse_relative_time_at_date()
created achrono::Duration
. Unfortunately,Duration
doesn't understand months and years, only seconds and nanoseconds. Therefore, the old code would assume every month was 30 days and every year was 365 days. This meant stuff liketouch -d "month"
wouldn't work in February andtouch -d "year"
wouldn't work in 2024.I've now made it so that
parse_relative_time_at_date
returns aDateTime
rather than aDuration
. Within that function,checked_(add/sub)_(days/months/signed)
is used to add days, months, years, whatever while being aware of the fact that months and years can be different sizes.parse_datetime_at_date()
in lib.rs now directly calls this function, so there's no need to calculate aDuration
at all.The old
parse_relative_time
function has now been relegated to being a helper for the relative time parsing tests.Testing
I haven't run all the uutils/coreutils tests yet, but I did run the following locally (I ran it between UTC midnight and my midnight). The
yesterday
bug mentioned in uutils/coreutils#6629 were not triggered.And here it is for
-d "month"
: