Skip to content

Impl time crate into pyo3 #5057

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

Merged
merged 17 commits into from
Apr 14, 2025
Merged

Conversation

Owen-CH-Leung
Copy link
Contributor

@Owen-CH-Leung Owen-CH-Leung commented Apr 8, 2025

FIxes #4675

This MR integrates time crate into PyO3 ecosystem. It implements FromPyObject and IntoPyObject for the following structs from time crate:

  • Date
  • Duration
  • Time
  • OffsetDateTime
  • PrimitiveDateTime
  • UtcOffset

@Owen-CH-Leung Owen-CH-Leung marked this pull request as ready for review April 8, 2025 12:28
@bschoenmaeckers
Copy link
Member

There is no implementation for UtcDateTime. Is that a oversight?

@Owen-CH-Leung
Copy link
Contributor Author

There is no implementation for UtcDateTime. Is that a oversight?

I provided an implementation in earlier commits but I realized that the time crate 0.3.41 will break the msrv (since the time-core requires minimum rust version 1.67.1)

https://github.com/time-rs/time/blob/v0.3.41/time-core/Cargo.toml

I duno if breaking msrv is a good idea so I decided to downgrade time crate back to 0.3.20 to meet the msrv requirement. And then in 0.3.20 the struct UtcDateTime didn't exist so I've removed the impl.

Should I just bump the msrv and bring back UtcDateTime ?

@bschoenmaeckers
Copy link
Member

It is OK to break the msrv in a optional feature. The jiff integration currently does the same thing (see #4823). Just make sure it is documented properly.

Further more forcing the time dependency to a fixed version is probably too invasive for users to be usable. So setting the minimum version to 0.3.38 should be sufficient.

@bschoenmaeckers
Copy link
Member

Please document the new feature in the guide as well; https://pyo3.rs/latest/features.html.

@Owen-CH-Leung
Copy link
Contributor Author

Owen-CH-Leung commented Apr 10, 2025

looks like main is broken ? @davidhewitt

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Thanks! #5065 should have unblocked CI.

Similar to the addition of jiff you have to add the time feature in the noxfile so that the tests run in CI (because it can't be in full for MSRV reasons)

Comment on lines 455 to 466
// // Get UTC timezone
// #[cfg(not(Py_LIMITED_API))]
// let py_tzinfo = py
// .import("datetime")?
// .getattr(intern!(py, "timezone"))?
// .getattr(intern!(py, "utc"))?;
//
// #[cfg(Py_LIMITED_API)]
let py_tzinfo = py
.import("datetime")?
.getattr(intern!(py, "timezone"))?
.getattr(intern!(py, "utc"))?;
Copy link
Member

Choose a reason for hiding this comment

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

Can should use the utc_timezone function. https://pyo3.rs/main/doc/pyo3/types/fn.timezone_utc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've switched to use timezone_utc function

Comment on lines 510 to 518
let is_utc = tzinfo
.call_method1(
"__eq__",
(ob.py()
.import("datetime")?
.getattr(intern!(ob.py(), "timezone"))?
.getattr(intern!(ob.py(), "utc"))?,),
)?
.extract::<bool>()?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let is_utc = tzinfo
.call_method1(
"__eq__",
(ob.py()
.import("datetime")?
.getattr(intern!(ob.py(), "timezone"))?
.getattr(intern!(ob.py(), "utc"))?,),
)?
.extract::<bool>()?;
let is_utc = tzinfo.eq(timezone_utc(py))?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks & changed.

Copy link
Member

@bschoenmaeckers bschoenmaeckers 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

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

Agreed! Thanks again 🚀

@Icxolu Icxolu enabled auto-merge April 14, 2025 16:03
@Icxolu Icxolu added this pull request to the merge queue Apr 14, 2025
Merged via the queue into PyO3:main with commit 73136ea Apr 14, 2025
43 of 44 checks passed
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.

Support time crate in addition to chrono for datetime types.
3 participants