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

ffi: support PyDateTime_TimeZone_UTC #1573

Merged
merged 1 commit into from Apr 29, 2021
Merged

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Apr 21, 2021

This adds a symbol for PyDateTime_TimeZone_UTC, as discussed in #207.

I made it work like PyDateTimeAPI, where it has a Deref 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.

@pickfire
Copy link
Contributor

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?

@davidhewitt
Copy link
Member Author

Just wondering, what are the other approaches?

Well, for example we could make some kind of static mut global which would be more similar to the underlying C API, but that'd be quite fiddly for users to handle safely!

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@pickfire
Copy link
Contributor

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 pyclass_slots but I only see __dict__ and __weakref__ there but not __slots__, am I missing something here?

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?

@davidhewitt
Copy link
Member Author

davidhewitt commented Apr 29, 2021

I think you should be aiming for a type similar to PyList defined in src/types/list.rs, where the pyobject_native_var_type! macro is used to set up all the right trait implementations.

pyobject_native_var_type isn't exactly correct (the type is fixed size) but it'll avoid you having to describe the C layout of the type.

That should be enough for getting a type which can be used to implement methods and conversions with.

@davidhewitt davidhewitt merged commit c0ff97e into PyO3:main Apr 29, 2021
@davidhewitt davidhewitt deleted the utc branch April 29, 2021 22:00
@pickfire
Copy link
Contributor

But PyTimeZone does not have a C type, from what I see the type is being defined in python library. Which is why I am confused, like what do I put for ffi::XXXType for pyobject_native_var_type!?

@davidhewitt
Copy link
Member Author

Ah, I'd completely missed that was the case. In which case I think that we can just refer to the existing PyTzInfo type (which is the base class of datetime.timezone) - that should be good enough for now?

I opened #1588 - do you think that'll resolve what you need?

Also, regarding chrono implementation... I kind of wonder if it would be easier for the community if the conversions lived here in src/conversions/chrono.rs, as an optional feature? Depends if you think the progress of implementing them in chrono is going anywhere.

@pickfire
Copy link
Contributor

pickfire commented May 1, 2021

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 impl PyTzInfo but I guess it is not necessary. I guess that's all I need, thanks for the quick response.

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

3 participants