Skip to content
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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pitdicker
Copy link
Collaborator

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".

  • The panic message is now "date outside of defined range for rfc2822".
  • I added a new method DateTime::try_to_rfc2822 that can return None when formatting fails.
  • DateTime::to_rfc2822 is deprecated with the notice: "Can panic on years outside of the range 0..=9999. Use try_to_rfc2822() instead."

Formatting with the RFC2822 formatting item could return a panic in the Display method of DelayedFormat.

  • The formatting item can be localized, making it already not fully correct according to RFC 2822. But it is convenient, and we have a dedicated method, so I kept it that way.
  • Formatting is extended to support years outside the range defined in RFC 2822 to prevent an error. It is now up to the calling function to ensure the date is in range.

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 by DateTime::parse_from_rfc3339.

  • to_rfc3339 now panics on out-of-range dates with "date outside of defined range for rfc3339", just like to_rfc2822.
  • A new try_to_rfc3339 can return None when formatting fails.
  • A new method to_iso8601 functions exactly as to_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. Use try_to_rfc3339() or to_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.
  • The 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 the RFC2822 formatting item: to not panic inside the Display implementation of DelayedFormat).
  • This is the formatting item for "%+".

ISO 8601 and usually valid RFC 3339 is also the Debug output of DateTime.

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 and IsoYearMod100. The implementation supports formatting dates outside the 0..=9999 range, which is now reflected in the documentation.

@pitdicker pitdicker force-pushed the strict_rfc_dates branch 2 times, most recently from 6eb97a4 to b22b0e9 Compare June 10, 2023 11:17
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jtmoon79 jtmoon79 left a 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.

src/datetime/mod.rs Outdated Show resolved Hide resolved
@pitdicker pitdicker force-pushed the strict_rfc_dates branch 2 times, most recently from d321c9b to e93a9be Compare July 24, 2023 07:52
@pitdicker
Copy link
Collaborator Author

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.

Thank you for even writing out some test cases.

@pitdicker pitdicker force-pushed the strict_rfc_dates branch 3 times, most recently from 2a458ab to 488571c Compare July 25, 2023 04:30
src/datetime/tests.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #1144 (4bc5075) into 0.4.x (604b92a) will increase coverage by 0.01%.
The diff coverage is 97.00%.

@@            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     
Files Coverage Δ
src/datetime/tests.rs 96.84% <100.00%> (+0.04%) ⬆️
src/format/mod.rs 85.04% <ø> (ø)
src/lib.rs 95.62% <100.00%> (+0.03%) ⬆️
src/offset/fixed.rs 82.35% <100.00%> (ø)
src/format/formatting.rs 92.43% <80.00%> (-0.04%) ⬇️
src/datetime/mod.rs 89.44% <95.23%> (+0.14%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jtmoon79 jtmoon79 left a 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/datetime/mod.rs Outdated Show resolved Hide resolved
src/format/mod.rs Show resolved Hide resolved
/// # Errors
///
/// RFC 2822 is only defined on years 0 through 9999, and returns an error on dates outside this
/// range.
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

@jtmoon79 jtmoon79 Sep 26, 2023

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 using format_localized I think the RFC2822 item should use English names as defined in the standard.

I agree: follow the standards.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

src/datetime/mod.rs Show resolved Hide resolved
src/datetime/mod.rs Show resolved Hide resolved
src/datetime/mod.rs Outdated Show resolved Hide resolved
src/datetime/mod.rs Outdated Show resolved Hide resolved
src/datetime/mod.rs Show resolved Hide resolved
src/datetime/mod.rs Show resolved Hide resolved
@pitdicker pitdicker mentioned this pull request Aug 27, 2023
@pitdicker pitdicker force-pushed the strict_rfc_dates branch 2 times, most recently from 0a73c65 to 408daa3 Compare September 2, 2023 06:13
@pitdicker
Copy link
Collaborator Author

@jtmoon79 Sorry it took so long, thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants