-
Notifications
You must be signed in to change notification settings - Fork 796
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
Update to chrono 0.4.34 #5385
Update to chrono 0.4.34 #5385
Conversation
@@ -115,6 +115,8 @@ jobs: | |||
uses: ./.github/actions/setup-builder | |||
- name: Install cargo-msrv | |||
run: cargo install cargo-msrv | |||
- name: Downgrade arrow dependencies | |||
run: cargo update -p ahash --precise 0.8.7 |
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.
ahash 0.8.8 bumped the MSRV to 1.72
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.
Thanks @tustvold
@@ -1013,9 +1013,9 @@ mod tests { | |||
assert_eq!(pretty[2], "0 days 0 hours 0 mins 0.000001000 secs"); | |||
assert_eq!(iso[3], "-PT0.000001S"); | |||
assert_eq!(pretty[3], "0 days 0 hours 0 mins -0.000001000 secs"); | |||
assert_eq!(iso[4], "P45DT50554.123456789S"); | |||
assert_eq!(iso[4], "PT3938554.123456789S"); |
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.
why did these values change? They look closer to the pretty version with the former formatting (like 45 days)
Is this something we need to change in pretty print?
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.
As per the PR description - chronotope/chrono#1328 changed the formatting to be more "correct". As the pretty version is focused on human readability, not precision, I am not sure it needs to be updated
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.
Makes sense
@@ -1192,7 +1192,7 @@ mod tests { | |||
|
|||
assert_json_eq( | |||
&buf, | |||
r#"{"duration_sec":"PT120S","duration_msec":"PT0.120S","duration_usec":"PT0.000120S","duration_nsec":"PT0.000000120S","name":"a"} | |||
r#"{"duration_sec":"PT120S","duration_msec":"PT0.12S","duration_usec":"PT0.00012S","duration_nsec":"PT0.00000012S","name":"a"} |
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.
this looks better
Which issue does this PR close?
Closes #.
Rationale for this change
chronotope/chrono#1328 changed chrono to no longer convert durations to a number of days when printing in the ISO compatible format.
What changes are included in this PR?
Are there any user-facing changes?
Technically maybe, although it wasn't a breaking chrono release so I'm not sure...