add _bound constructors for datetime types
#3778
Conversation
davidhewitt
force-pushed
the
datetime-bound-constructors
branch
from
0ae7da3 to
ad80c94
Compare
|
Ok, I think this one should now be good to review! 🎉 |
CodSpeed Performance ReportMerging #3778 will not alter performanceComparing Summary
|
pytests/src/datetime.rs
Outdated
| tzinfo: Option<&PyTzInfo>, | ||
| ) -> PyResult<&'p PyTime> { | ||
| PyTime::new(py, hour, minute, second, microsecond, tzinfo) | ||
| tzinfo: Option<&Bound<'_, PyTzInfo>>, |
There was a problem hiding this comment.
I assume the compiler is smart enough to figure out that the inner lifetime here has to be 'py, but maybe its nicer for consistency to explicitly name it if we have it named anyway.
There was a problem hiding this comment.
Agreed, I had been tempted to leave these out as it didn't feel strictly necessary. On refection just having 'py consistently added to all Bound (if the lifetime is named) is a simple rule which makes readable code 👍
I'll update the whole file.
src/ffi/tests.rs
Outdated
| @@ -79,7 +79,7 @@ fn test_timezone_from_offset() { | |||
| use crate::types::PyDelta; | |||
|
|
|||
| Python::with_gil(|py| { | |||
| let delta = PyDelta::new(py, 0, 100, 0, false).unwrap(); | |||
| let delta = PyDelta::new_bound(py, 0, 100, 0, false).unwrap(); | |||
| let tz: &PyAny = unsafe { py.from_borrowed_ptr(PyTimeZone_FromOffset(delta.as_ptr())) }; | |||
There was a problem hiding this comment.
Maybe we can remove this gil-ref (and the one below) as well, while we're here.
There was a problem hiding this comment.
Good idea. Reading this code I see they're also bugged. PyTimeZone_FromOffset is documented to return a new reference, so I'll change this to .assume_owned(py).
davidhewitt
force-pushed
the
datetime-bound-constructors
branch
from
a45857c to
10cb090
Compare
|
Thanks for the thorough review! In the spirit of #3811 I've chosen to push the review feedback as new commits to make it easier to see what I've updated. |
davidhewitt
force-pushed
the
datetime-bound-constructors
branch
from
1ce1393 to
43217dd
Compare
For #3684
This PR adds
new_boundand variants for thePyDate,PyDateTime,PyTimeandPyDeltatypes.Also adds
timezone_utc_bound.Updates internal code to use these (basically the
chronoconversions).WIP - the
pytestschanges as written don't compile until #3776 allows&Bound<T>as a function argument.