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

Fallible variants for TimeZone::from_utc_* #1499

Closed
wants to merge 4 commits into from

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Mar 9, 2024

As discussed in #1448 (comment):

What would we need to change to propagate the error instead of unwrapping on this line in Local::offset_from_utc_datetime?

Three methods:

  • TimeZone::offset_from_utc_datetime
  • Timezone::from_utc_datetime
  • DateTime::with_timezone

Those methods currently come with the assumption that finding the offset of a UTC datetime in some time zone will never fail. This is reasonable in a mathematical sense, but ignores that a TimeZone implementation can run into errors, such as our Local type or a more flexible type that uses the OS time zone information.

Within chrono it turns out these methods are used almost always in methods that are already fallible. Or, for DateTime::with_timezone, there are the infallible DateTime::to_utc and DateTime::fixed_offset methods which cover many uses.

The first commit adds TimeZone::offset_from_utc_datetime_opt, Timezone::from_utc_datetime_opt and DateTime::with_timezone_opt.
The second makes our platform implementations for Local return an Option (instead of going through LocalResult).

The third commit goes through all uses of the existing three methods and changes them to propagate the error, or switch to the infallible DateTime::to_utc and DateTime::fixed_offset. In cases where we still need to panic I used expect instead of hiding the panic case behind another layer.

For the rest I adjusted the tests to make it easy to switch to fallible methods on the 0.5 branch.

Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 90.32258% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 91.78%. Comparing base (1789183) to head (84806bd).

Files Patch % Lines
src/datetime/mod.rs 61.29% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1499      +/-   ##
==========================================
- Coverage   91.80%   91.78%   -0.03%     
==========================================
  Files          40       40              
  Lines       18338    18369      +31     
==========================================
+ Hits        16836    16860      +24     
- Misses       1502     1509       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pitdicker pitdicker force-pushed the fallible_from_utc branch 2 times, most recently from c1ccc1a to dd9933c Compare March 9, 2024 11:48
@pitdicker pitdicker marked this pull request as ready for review March 9, 2024 11:49
/// from for example an OS API, in which case this method returns `None`.
#[inline]
#[must_use]
pub fn with_timezone_opt<Tz2: TimeZone>(&self, tz: &Tz2) -> Option<DateTime<Tz2>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something I've been thinking about: for newly fallible API, shouldn't we directly start to yield some kind of Result instead of sticking with Option everywhere?

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 sometimes think the same. On the other hand it would make the API seem even less consistent if we end up with a handful of methods that return Result and the rest returns Option.

@pitdicker
Copy link
Collaborator Author

I feel like these changes add friction to the 0.4 versions for little benefit.
Our implementation evolved to be infallible by either falling back to UTC (except for bugs), or returning LocalResult::None. That does not seem like something to just change in a minor version. So the new fallible methods should pretty much always return Ok.

I'd like to only do this on the 0.5 branch and directly return an error type.

@pitdicker pitdicker closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants