Various fixes for formatting dates with a year outside of 0..=9999#1144
Various fixes for formatting dates with a year outside of 0..=9999#1144pitdicker wants to merge 7 commits intochronotope:mainfrom
Conversation
6eb97a4 to
b22b0e9
Compare
418c3be to
0898c39
Compare
There was a problem hiding this comment.
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
jtmoon79
left a comment
There was a problem hiding this comment.
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.
Perhaps mention
/// Uses `default_locale()` for locale information.
There was a problem hiding this comment.
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.
write_rfc2822_innerlocalizes the day and month.
Even while usingformat_localizedI think the RFC2822 item should use English names as defined in the standard.
I agree: follow the standards.
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_rfc2822panicked with "writing rfc2822 datetime to string should never fail".DateTime::try_to_rfc2822that can returnNonewhen formatting fails.DateTime::to_rfc2822is deprecated with the notice: "Can panic on years outside of the range 0..=9999. Usetry_to_rfc2822()instead."Formatting with the
RFC2822formatting item could return a panic in theDisplaymethod 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_rfc3339was 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_rfc3339now panics on out-of-range dates with "date outside of defined range for rfc3339", just liketo_rfc2822.try_to_rfc3339can returnNonewhen formatting fails.to_iso8601functions exactly asto_rfc3339did, because dates that are out of range for RFC 3339 are still valid ISO 8601.to_rfc3339is deprecated with the notice: "Can panic on years outside of the range 0..=9999. Usetry_to_rfc3339()orto_iso8601instead.". This way when users update chrono to a new version a panic is not silently introduced.Other RFC 3339 formatting methods:
DateTime::to_rfc3339_optsis redefined to format to an ISO 8601 representation when the year is out of range.RFC3339formatting item is also redefined to format to an ISO 8601 representation when the year is out of range (for the same reasons as theRFC2822formatting item: to not panic inside theDisplayimplementation ofDelayedFormat)."%+".ISO 8601 and usually valid RFC 3339 is also the
Debugoutput 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,IsoYearDiv100andIsoYearMod100. The implementation supports formatting dates outside the 0..=9999 range, which is now reflected in the documentation.