Skip to content

Conversation

@sorairolake
Copy link
Contributor

We should use TryFrom<PrimitiveDateTime> for DateTime or TryFrom<DateTime> for PrimitiveDateTime instead of these deprecated methods. This is because, unlike OffsetDateTime, DateTime and PrimitiveDateTime does not hold a time zone.

  • DateTime::from_time has been deprecated since version 0.6.4, released in February 2023.
  • DateTime::to_time has been deprecated since version 1.3.1, released in May 2024.
  • TryFrom<PrimitiveDateTime> for DateTime and TryFrom<DateTime> for PrimitiveDateTime are available in version 2.5.0 or later, released in March 2025.

Important

This is a breaking change, so it cannot be merged until the next major version.

See also #321

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @sorairolake, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request streamlines the DateTime API by removing several deprecated methods and associated TryFrom implementations that handled conversions with time::OffsetDateTime. The rationale behind these removals is to align DateTime and PrimitiveDateTime with their intended design of not holding timezone information, encouraging the use of newer, more appropriate TryFrom implementations available in the time crate. This is a breaking change, preparing the library for a future major version update.

Highlights

  • Removal of Deprecated Methods: The DateTime::from_time and DateTime::to_time methods have been removed. These methods were previously marked as deprecated in versions 0.6.4 and 1.3.1, respectively, with notes to use DateTime::try_from() or time::OffsetDateTime::try_from() instead.
  • Removal of TryFrom Implementations: The TryFrom<time::OffsetDateTime> for DateTime and TryFrom<DateTime> for time::OffsetDateTime implementations have been removed. The rationale is that DateTime and PrimitiveDateTime do not hold timezone information, unlike OffsetDateTime.
  • Test Cleanup: Corresponding tests for the removed methods and TryFrom implementations, including datetime_try_from_offset_datetime, offset_datetime_try_from_datetime, and offset_datetime_try_from_bounds, have been removed. Additionally, #[allow(deprecated)] attributes in other tests were removed as they are no longer necessary.
  • Breaking Change: This change is considered a breaking change due to the removal of public API and is intended for the next major version release, aligning with the availability of newer TryFrom implementations in time crate version 2.5.0 or later.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request successfully removes the deprecated from_time and to_time methods from the DateTime struct, along with their corresponding TryFrom implementations for time::OffsetDateTime and related tests. This aligns with the stated goal of simplifying DateTime by removing functionality that incorrectly implies timezone awareness, as DateTime and PrimitiveDateTime do not inherently handle time zones. The changes are consistent and contribute to a cleaner API, as noted in the pull request description regarding it being a breaking change for the next major version.

@Pr0methean Pr0methean added this to the 8.0.0 milestone Jan 30, 2026
Pr0methean
Pr0methean previously approved these changes Jan 30, 2026
Updated comment for deprecated-time feature to clarify its purpose and future removal.

Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Pr0methean
Pr0methean previously approved these changes Jan 30, 2026
Updated comment for deprecated-time feature in Cargo.toml.

Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
@Its-Just-Nans
Copy link
Member

Hi

What the goal of introducing the deprecated-time feature?

Since it's there is always the time feature enabled

@Pr0methean
Copy link
Member

Pr0methean commented Jan 30, 2026

Hi

What the goal of introducing the deprecated-time feature?

Since it's there is always the time feature enabled

Because the time feature will still have functionality that isn't deprecated, namely the conversions to/from PrimitiveDateTime. The point of deprecated-time is to ensure direct consumers of zip 8.*.* who convert a zoneless date and time to/from OffsetDateTime, and transitive consumers who audit their cargo tree, are aware they're using functionality that will eventually be removed, and therefore that they'd benefit from migrating to PrimitiveDateTime while they still have time, if they're to still have time.

@Its-Just-Nans
Copy link
Member

So if I understand, this is for user which uses

zip crate with the time feature enabled
It also enables deprecated-time
But deprecated-time doesn't add anything no?

I think #[cfg(all(feature = "time", feature = "deprecated-time"))] should just be #[cfg(feature = "deprecated-time")]

no? Am I missing something?

@Pr0methean
Copy link
Member

Pr0methean commented Jan 30, 2026

So if I understand, this is for user which uses

zip crate with the time feature enabled It also enables deprecated-time But deprecated-time doesn't add anything no?

Not true: time without deprecated-time will enable conversion to/from time::PrimitiveDateTime. time with deprecated-time will also enable conversion to/from time::OffsetDateTime even though ZIP's timestamps don't have a time zone and OffsetDateTime does.

I think #[cfg(all(feature = "time", feature = "deprecated-time"))] should just be #[cfg(feature = "deprecated-time")]

no? Am I missing something?

deprecated-time doesn't imply time because we want the dependencies that will still be present in 9.0.0 to be explicit. And anyone who depends on the deprecated-time feature will still depend on our time feature once they migrate from time::OffsetDateTime to time::PrimitiveDateTime as we recommend (unless they specifically choose to switch to chrono), so the current PR's configuration ensures they won't be able to build against 8.0.0 without the time feature and then unexpectedly need to build against 9.0.0 with the time feature. Since the only changes they've made will have been to replace deprecated type conversions with their closest replacements, the only Cargo.toml change they have to make should be removing the feature flag with a deprecated-* name that enabled those specific conversions.

@Its-Just-Nans
Copy link
Member

Not true

Okay now I understand what I was missing, it's a [cfg(all(...))] and not [cfg(any(...))]. I missread, sorry 😅

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.

3 participants