-
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
Make PyDateTime::new
take a &PyTzInfo
#392
Comments
@konstin One thing we may want to take into account would be the possible performance implications of this, I think. I'm not sure if Unfortunately with all the procedural macros (and the fact that the overhead has not been optimized too much), it's hard for me to tell how much overhead we have on the existing functions anyway, and how much this will change it. |
Oh, that's a good point. The conversion is not zero cost, because we need to make one call to cpython's type check method. But I don't think we should care about the performance impact for now as we don't have benchmarks nor too many optimizations. |
Thinking about this more, the requirement that something be a pub fn new<TZ: IntoPyTzInfo, 'p> (
py: Python<'p>,
year: i32,
...
tzinfo: Option<TZ>,
) -> PyResult<&'p PyDateTime>
{ The C API enforces that whatever's there is a |
I just realized the downside of the above is that it will require an explicit reference to PyDateTime::new(2019, 1, 1, 0, 0, 0, 0, None) will have to become: PyDateTime::new::<PyTzInfo>(2019, 1, 1, 0, 0, 0, None) which I don't love. Maybe the best option is to switch it over to |
Using a trait sounds great! We might be able to solve the |
@konstin I wasn't aware of defaulted generic parameters, thanks for the pointer. Apparently, they are not valid for function definitions, though (the compiler points to this issue), which means we can't use them for the constructors. I'm still kinda in favor of using |
Closing; in #1588 I changed |
Change the argument of
PyDateTime::new
to take aOption<&PyTzInfo>
instead of aOption<&PyObject>
as per discussion in #377 (comment).The text was updated successfully, but these errors were encountered: