Don't use Durations for calculating relative times#85
Merged
cakebaker merged 6 commits intouutils:mainfrom Sep 17, 2024
ysthakur:fix-relative-time
Merged
Don't use Durations for calculating relative times#85cakebaker merged 6 commits intouutils:mainfrom ysthakur:fix-relative-time
cakebaker merged 6 commits intouutils:mainfrom
ysthakur:fix-relative-time
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. |
Krysztal112233
approved these changes
Sep 5, 2024
Krysztal112233
left a comment
There was a problem hiding this comment.
Seems good.
Could you please review this pull request? @cakebaker
Contributor
Author
|
@cakebaker Could you take a look at this if you have time? |
Collaborator
|
Good work, thanks for your PR! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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,Durationdoesn'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_datereturns aDateTimerather 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 aDurationat all.The old
parse_relative_timefunction 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
yesterdaybug mentioned in uutils/coreutils#6629 were not triggered.And here it is for
-d "month":