-
Notifications
You must be signed in to change notification settings - Fork 533
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
Various fixes for formatting dates with a year outside of 0..=9999 #1144
base: main
Are you sure you want to change the base?
Conversation
6eb97a4
to
b22b0e9
Compare
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.
LGTM
418c3be
to
0898c39
Compare
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.
I strongly recommend some tests to confirm the panic. It's an important behavior presumption to verify as it has a large affect on the important parts of chrono. e.g.
#[should_panic]
fn test_rfc_2822_year_range_panic() {
use chrono::{TimeZone, Utc};
Utc.with_ymd_and_hms(10000, 1, 2, 3, 4, 5).unwrap().to_rfc2822();
}
Same test case for RFC 3339.
It'd also be good to add global constants for that range, e.g.
const RFC_2822_YEAR_MAX = 9999;
const RFC_2822_YEAR_MIN = 0;
Add the same constants for RFC 3339.
Then I'd improve the prior test case to use the constants, and add more test cases to test at the boundaries.
fn test_rfc_2822_year_range_max() {
use chrono::{TimeZone, Utc};
Utc.with_ymd_and_hms(RFC_2822_YEAR_MAX, 1, 2, 3, 4, 5).unwrap();
assert_eq!(dt.to_rfc2822(), "Sat, 2 Jan 9999 03:04:05 +0000");
}
fn test_rfc_2822_year_range_min() {
use chrono::{TimeZone, Utc};
Utc.with_ymd_and_hms(RFC_2822_YEAR_MIN, 1, 2, 3, 4, 5).unwrap();
assert_eq!(dt.to_rfc2822(), "Sat, 2 Jan 0000 03:04:05 +0000");
}
#[should_panic]
fn test_rfc_2822_year_range_panic_over_max() {
use chrono::{TimeZone, Utc};
Utc.with_ymd_and_hms(RFC_2822_YEAR_MAX + 1, 1, 2, 3, 4, 5).unwrap().to_rfc2822();
}
#[should_panic]
fn test_rfc_2822_year_range_panic_under_min() {
use chrono::{TimeZone, Utc};
Utc.with_ymd_and_hms(RFC_2822_YEAR_MIN - 1, 1, 2, 3, 4, 5).unwrap().to_rfc2822();
}
(I don't know of Sat
is the correct day of week for my code examples)
Then add the same test cases for RFC 3339.
d321c9b
to
e93a9be
Compare
Thank you for even writing out some test cases. |
2a458ab
to
488571c
Compare
488571c
to
4fd4e8f
Compare
Codecov Report
@@ Coverage Diff @@
## 0.4.x #1144 +/- ##
==========================================
+ Coverage 91.50% 91.52% +0.01%
==========================================
Files 38 38
Lines 17314 17371 +57
==========================================
+ Hits 15844 15898 +54
- Misses 1470 1473 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
4fd4e8f
to
2424753
Compare
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.
Approved with feedback about various docstring oddities and suggestions.
src/format/formatting.rs
Outdated
/// # Errors | ||
/// | ||
/// RFC 2822 is only defined on years 0 through 9999, and returns an error on dates outside this | ||
/// range. |
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.
Perhaps mention
/// Uses `default_locale()` for locale information.
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.
I am a bit uncomfortable with the fact that write_rfc2822_inner
localizes the day and month.
Even while using format_localized
I think the RFC2822
item should use English names as defined in the standard.
@djc Shall I remove localization from write_rfc2822
? It lives behind the unstable-locales
feature, so making a change in behaviour seems ok.
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.
write_rfc2822_inner
localizes the day and month.
Even while usingformat_localized
I think the RFC2822 item should use English names as defined in the standard.
I agree: follow the standards.
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.
Agreed.
0a73c65
to
408daa3
Compare
@jtmoon79 Sorry it took so long, thank you for the review! |
2dba78d
to
17ba472
Compare
17ba472
to
82036c4
Compare
75b9874
to
69ed1b5
Compare
69ed1b5
to
9e68997
Compare
9e68997
to
4bc5075
Compare
We have a couple of problems when formatting dates with a year outside of 0..=9999.
RFC 2822
Formatting in
DateTime::to_rfc2822
panicked with "writing rfc2822 datetime to string should never fail".DateTime::try_to_rfc2822
that can returnNone
when formatting fails.DateTime::to_rfc2822
is deprecated with the notice: "Can panic on years outside of the range 0..=9999. Usetry_to_rfc2822()
instead."Formatting with the
RFC2822
formatting item could return a panic in theDisplay
method ofDelayedFormat
.RFC 3339 and ISO 8601
RFC 3339 is not defined for dates with a year outside of 0..=9999. ISO 8601 does support it.
DateTime::to_rfc3339
was a hybrid method, claiming to format a datetime as both valid RFC 3339 and ISO 8601. It would write invalid RFC 3339 strings if the year is out of range, which could not be parsed byDateTime::parse_from_rfc3339
.to_rfc3339
now panics on out-of-range dates with "date outside of defined range for rfc3339", just liketo_rfc2822
.try_to_rfc3339
can returnNone
when formatting fails.to_iso8601
functions exactly asto_rfc3339
did, because dates that are out of range for RFC 3339 are still valid ISO 8601.to_rfc3339
is deprecated with the notice: "Can panic on years outside of the range 0..=9999. Usetry_to_rfc3339()
orto_iso8601
instead.". This way when users update chrono to a new version a panic is not silently introduced.Other RFC 3339 formatting methods:
DateTime::to_rfc3339_opts
is redefined to format to an ISO 8601 representation when the year is out of range.RFC3339
formatting item is also redefined to format to an ISO 8601 representation when the year is out of range (for the same reasons as theRFC2822
formatting item: to not panic inside theDisplay
implementation ofDelayedFormat
)."%+"
.ISO 8601 and usually valid RFC 3339 is also the
Debug
output ofDateTime
.We have no parser that can read these out-of-range dates, which is what #1143 is for.
Year formatting items
I have updated the documentation of the formatting items
YearDiv100
,YearMod100
,IsoYearDiv100
andIsoYearMod100
. The implementation supports formatting dates outside the 0..=9999 range, which is now reflected in the documentation.