-
Notifications
You must be signed in to change notification settings - Fork 739
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
ffi: support PyDateTime_TimeZone_UTC #1573
Conversation
e0a9698
to
d06764d
Compare
Nice, thanks a lot for showing me how to do this, I probably will get stuck forever on the UTC thing if I try it myself especially with the 3 cast. Just wondering, what are the other approaches? |
Well, for example we could make some kind of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you!
https://github.com/python/cpython/blob/3.9/Lib/datetime.py#L2217 class timezone(tzinfo):
__slots__ = '_offset', '_name' I am not sure how to implement slots within pyo3 src/types/datetime. From what I see we don't have any places that have a subclass of an existing python type which is a wrapper in rust for me to reference. I saw architecture.md and Also, am I supposed to do // src/types/datetime.rs
#[pyclass(extends=PyTzInfo)]
pub struct PyTimeZone {
_offset: *mut PyObject,
_name: *mut PyObject,
} Or maybe am I doing it wrongly? |
I think you should be aiming for a type similar to
That should be enough for getting a type which can be used to implement methods and conversions with. |
But |
Ah, I'd completely missed that was the case. In which case I think that we can just refer to the existing I opened #1588 - do you think that'll resolve what you need? Also, regarding |
Chrono implementation is just missing the datetime, putting it in chrono pros is that we can use the internal representation directly and that would probably be faster if the compiler is not able to optimize it. But the cons is that it is troublesome without bumping the compiler version, maybe just for the feature it can have a higher compiler version. So far I am stuck because chrono is missing PyTzInfo (including FixedOffset) and UTC, I wanted to try adding it here but seemed harder than I thought. Yup, the patch that you send seemed a lot better. I thought I need to |
This adds a symbol for
PyDateTime_TimeZone_UTC
, as discussed in #207.I made it work like
PyDateTimeAPI
, where it has aDeref
impl which in this case yields&'static PyObject
. I'm slightly split on whether I like this approach; in particular because it can run a Python import in the background on first use. On the other hand, it's safe and convenient for users.TODO: add a test.