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

Add chrono support #2604

Closed
wants to merge 2 commits into from
Closed

Add chrono support #2604

wants to merge 2 commits into from

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Aug 31, 2022

Moved from chronotope/chrono#542

What's left

cc @Psykopear I just pushed so that you can help review even though it's not complete yet, but all the types are now there

Comment on lines +489 to +496
/// Equivalent to `datetime.timezone`
// Not making this public yet as the implementation is slightly different from
// original cpython.
// #[crate::pyclass]
// pub(crate) struct PyTimeZone {
// offset: PyDelta,
// // name
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking it is better to have our own timezone port, but I haven't figured out how to get pyclass working yet. Original source https://github.com/python/cpython/blob/29f1b0bb1ff73dcc28f0ca7e11794141b6de58c9/Lib/datetime.py#L2296-L2314

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same problem here, it's the first time I work in PyO3, but it seems those macros are never used inside the crate, I think this will have to be done without the macros

Copy link
Member

Choose a reason for hiding this comment

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

The problem with having a pyclass such as this is that they behave poorly when shared between libraries. See also #1444

Most likely you (or your users) will run into weird errors like TypeError: 'PyTimeZone' object cannot be converted to 'PyTimeZone'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just read that issue, it makes sense, so we won't have this here and we should ask users to implement the type in their own crate. We could add the example to the docs pointing at that issue, so that if it's fixed in the future we can move the PyClass here.

@Psykopear
Copy link
Contributor

Psykopear commented Aug 31, 2022

That's great, thank you!
A couple of things:

  • You mention the need for an update on the time crate, to get the SECS_PER_DAY const, but the const is defined inside chrono itself (in src/time_delta.rs), which doesn't depend on time anymore. It's not public, but we can either just change that, or define it here, it's a fixed number and I hope it doesn't change (not being ironic, I just think I can't assume anything when talking about datetimes)
  • What you do here for the conversion can be done with the current chrono api, it's less efficient (using date.year() ecc., rather than exposing date.mdf()), but it would work without any change to chrono. It might make sense to make this PR work with the current version of chrono, and maybe update it once chrono exposes the needed api? I say this because I feel like the changes on chrono would not be trivial, they'd need to expose some structs/functions that are currently private, and this might be a long process. Also, it's should be an implementation detail here, updating it later won't change anything in the exposed functions/structs.

edit: I'll make a proper review of the code tomorrow morning, I'll also try it on the library I'm working on at the moment, where I have some tests for the python integration, and let you know if I have any problem

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off! Please feel free to ping me whenever you need reviews.

@@ -56,7 +58,7 @@ widestring = "0.5.1"
pyo3-build-config = { path = "pyo3-build-config", version = "0.17.1", features = ["resolve-config"] }

[features]
default = ["macros"]
default = ["macros", "chrono"]
Copy link
Member

Choose a reason for hiding this comment

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

When we get around to merging this we should remove it from the defaults here. Also mention in features.md.

@@ -0,0 +1,266 @@
#![cfg(feature = "chrono")]
Copy link
Member

Choose a reason for hiding this comment

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

If there are any Error types in chrono, it may be nice to also implement From<Error> for PyErr for them.

src/conversions/chrono.rs Outdated Show resolved Hide resolved
@Psykopear
Copy link
Contributor

@pickfire I opened a PR pickfire#1 over your fork, I updated this to work with the published version of chrono, without any change to the api. Let me know what you think, and if you like the idea feel free to merge it.

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

4 participants