Skip to content

Commit

Permalink
review: Icxolu feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@davidhewitt
davidhewitt committed Feb 11, 2024
1 parent f71e5f7 commit 10cb090
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 66 deletions.
24 changes: 12 additions & 12 deletions pytests/src/datetime.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ fn make_time<'py>(
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
tzinfo: Option<&Bound<'py, PyTzInfo>>,
) -> PyResult<Bound<'py, PyTime>> {
PyTime::new_bound(py, hour, minute, second, microsecond, tzinfo)
}
Expand All @@ -41,7 +41,7 @@ fn time_with_fold<'py>(
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
tzinfo: Option<&Bound<'py, PyTzInfo>>,
fold: bool,
) -> PyResult<Bound<'py, PyTime>> {
PyTime::new_bound_with_fold(py, hour, minute, second, microsecond, tzinfo, fold)
Expand Down Expand Up @@ -85,9 +85,9 @@ fn make_delta(
}

#[pyfunction]
fn get_delta_tuple<'py>(py: Python<'py>, delta: &Bound<'_, PyDelta>) -> Bound<'py, PyTuple> {
fn get_delta_tuple<'py>(delta: &Bound<'py, PyDelta>) -> Bound<'py, PyTuple> {
PyTuple::new_bound(
py,
delta.py(),
[
delta.get_days(),
delta.get_seconds(),
Expand All @@ -107,7 +107,7 @@ fn make_datetime<'py>(
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
tzinfo: Option<&Bound<'py, PyTzInfo>>,
) -> PyResult<Bound<'py, PyDateTime>> {
PyDateTime::new_bound(
py,
Expand All @@ -123,7 +123,7 @@ fn make_datetime<'py>(
}

#[pyfunction]
fn get_datetime_tuple<'py>(py: Python<'py>, dt: &Bound<'_, PyDateTime>) -> Bound<'py, PyTuple> {
fn get_datetime_tuple<'py>(py: Python<'py>, dt: &Bound<'py, PyDateTime>) -> Bound<'py, PyTuple> {
PyTuple::new_bound(
py,
[
Expand All @@ -141,7 +141,7 @@ fn get_datetime_tuple<'py>(py: Python<'py>, dt: &Bound<'_, PyDateTime>) -> Bound
#[pyfunction]
fn get_datetime_tuple_fold<'py>(
py: Python<'py>,
dt: &Bound<'_, PyDateTime>,
dt: &Bound<'py, PyDateTime>,
) -> Bound<'py, PyTuple> {
PyTuple::new_bound(
py,
Expand All @@ -162,18 +162,18 @@ fn get_datetime_tuple_fold<'py>(
fn datetime_from_timestamp<'py>(
py: Python<'py>,
ts: f64,
tz: Option<&Bound<'_, PyTzInfo>>,
tz: Option<&Bound<'py, PyTzInfo>>,
) -> PyResult<Bound<'py, PyDateTime>> {
PyDateTime::from_timestamp_bound(py, ts, tz)
}

#[pyfunction]
fn get_datetime_tzinfo(dt: &PyDateTime) -> Option<Bound<'_, PyTzInfo>> {
fn get_datetime_tzinfo<'py>(dt: &Bound<'py, PyDateTime>) -> Option<Bound<'py, PyTzInfo>> {
dt.get_tzinfo_bound()
}

#[pyfunction]
fn get_time_tzinfo(dt: &PyTime) -> Option<Bound<'_, PyTzInfo>> {
fn get_time_tzinfo<'py>(dt: &Bound<'py, PyTime>) -> Option<Bound<'py, PyTzInfo>> {
dt.get_tzinfo_bound()
}

Expand All @@ -190,7 +190,7 @@ impl TzClass {
fn utcoffset<'py>(
&self,
py: Python<'py>,
_dt: &Bound<'_, PyDateTime>,
_dt: &Bound<'py, PyDateTime>,
) -> PyResult<Bound<'py, PyDelta>> {
PyDelta::new_bound(py, 0, 3600, 0, true)
}
Expand All @@ -199,7 +199,7 @@ impl TzClass {
String::from("+01:00")
}

fn dst(&self, _dt: &Bound<'_, PyDateTime>) -> Option<&PyDelta> {
fn dst<'py>(&self, _dt: &Bound<'py, PyDateTime>) -> Option<Bound<'py, PyDelta>> {
None
}
}
Expand Down
13 changes: 5 additions & 8 deletions src/ffi/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,11 @@ fn test_utc_timezone() {
#[cfg(feature = "macros")]
#[cfg_attr(target_arch = "wasm32", ignore)] // DateTime import fails on wasm for mysterious reasons
fn test_timezone_from_offset() {
use crate::types::PyDelta;
use crate::{ffi_ptr_ext::FfiPtrExt, types::PyDelta};

Python::with_gil(|py| {
let delta = PyDelta::new_bound(py, 0, 100, 0, false).unwrap();
let tz: &PyAny = unsafe { py.from_borrowed_ptr(PyTimeZone_FromOffset(delta.as_ptr())) };
let tz = unsafe { PyTimeZone_FromOffset(delta.as_ptr()).assume_owned(py) };
crate::py_run!(
py,
tz,
Expand All @@ -95,16 +95,13 @@ fn test_timezone_from_offset() {
#[cfg(feature = "macros")]
#[cfg_attr(target_arch = "wasm32", ignore)] // DateTime import fails on wasm for mysterious reasons
fn test_timezone_from_offset_and_name() {
use crate::types::PyDelta;
use crate::{ffi_ptr_ext::FfiPtrExt, types::PyDelta};

Python::with_gil(|py| {
let delta = PyDelta::new_bound(py, 0, 100, 0, false).unwrap();
let tzname = PyString::new_bound(py, "testtz");
let tz: &PyAny = unsafe {
py.from_borrowed_ptr(PyTimeZone_FromOffsetAndName(
delta.as_ptr(),
tzname.as_ptr(),
))
let tz = unsafe {
PyTimeZone_FromOffsetAndName(delta.as_ptr(), tzname.as_ptr()).assume_owned(py)
};
crate::py_run!(
py,
Expand Down
108 changes: 62 additions & 46 deletions src/types/datetime.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,9 @@ impl PyDate {
pub fn new_bound(py: Python<'_>, year: i32, month: u8, day: u8) -> PyResult<Bound<'_, PyDate>> {
let api = ensure_datetime_api(py)?;
unsafe {
let ptr = (api.Date_FromDate)(year, c_int::from(month), c_int::from(day), api.DateType);
ptr.assume_owned_or_err(py).downcast_into_unchecked()
(api.Date_FromDate)(year, c_int::from(month), c_int::from(day), api.DateType)
.assume_owned_or_err(py)
.downcast_into_unchecked()
}
}

Expand All @@ -245,8 +246,9 @@ impl PyDate {
let _api = ensure_datetime_api(py)?;

unsafe {
let ptr = PyDate_FromTimestamp(time_tuple.as_ptr());
ptr.assume_owned_or_err(py).downcast_into_unchecked()
PyDate_FromTimestamp(time_tuple.as_ptr())
.assume_owned_or_err(py)
.downcast_into_unchecked()
}
}
}
Expand Down Expand Up @@ -300,17 +302,17 @@ impl PyDateTime {
)
)]
#[allow(clippy::too_many_arguments)]
pub fn new<'p>(
py: Python<'p>,
pub fn new<'py>(
py: Python<'py>,
year: i32,
month: u8,
day: u8,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&PyTzInfo>,
) -> PyResult<&'p PyDateTime> {
tzinfo: Option<&'py PyTzInfo>,
) -> PyResult<&'py PyDateTime> {
Self::new_bound(
py,
year,
Expand All @@ -327,17 +329,17 @@ impl PyDateTime {

/// Creates a new `datetime.datetime` object.
#[allow(clippy::too_many_arguments)]
pub fn new_bound<'p>(
py: Python<'p>,
pub fn new_bound<'py>(
py: Python<'py>,
year: i32,
month: u8,
day: u8,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
) -> PyResult<Bound<'p, PyDateTime>> {
tzinfo: Option<&Bound<'py, PyTzInfo>>,
) -> PyResult<Bound<'py, PyDateTime>> {
let api = ensure_datetime_api(py)?;
unsafe {
(api.DateTime_FromDateAndTime)(
Expand All @@ -357,19 +359,26 @@ impl PyDateTime {
}

/// Deprecated form of [`PyDateTime::new_bound_with_fold`].
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyDateTime::new_with_fold` will be replaced by `PyDateTime::new_bound_with_fold` in a future PyO3 version"
)
)]
#[allow(clippy::too_many_arguments)]
pub fn new_with_fold<'p>(
py: Python<'p>,
pub fn new_with_fold<'py>(
py: Python<'py>,
year: i32,
month: u8,
day: u8,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&PyTzInfo>,
tzinfo: Option<&'py PyTzInfo>,
fold: bool,
) -> PyResult<&'p PyDateTime> {
) -> PyResult<&'py PyDateTime> {
Self::new_bound_with_fold(
py,
year,
Expand All @@ -393,18 +402,18 @@ impl PyDateTime {
/// represented time is ambiguous.
/// See [PEP 495](https://www.python.org/dev/peps/pep-0495/) for more detail.
#[allow(clippy::too_many_arguments)]
pub fn new_bound_with_fold<'p>(
py: Python<'p>,
pub fn new_bound_with_fold<'py>(
py: Python<'py>,
year: i32,
month: u8,
day: u8,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
tzinfo: Option<&Bound<'py, PyTzInfo>>,
fold: bool,
) -> PyResult<Bound<'p, PyDateTime>> {
) -> PyResult<Bound<'py, PyDateTime>> {
let api = ensure_datetime_api(py)?;
unsafe {
(api.DateTime_FromDateAndTimeAndFold)(
Expand Down Expand Up @@ -432,23 +441,23 @@ impl PyDateTime {
note = "`PyDateTime::from_timestamp` will be replaced by `PyDateTime::from_timestamp_bound` in a future PyO3 version"
)
)]
pub fn from_timestamp<'p>(
py: Python<'p>,
pub fn from_timestamp<'py>(
py: Python<'py>,
timestamp: f64,
tzinfo: Option<&PyTzInfo>,
) -> PyResult<&'p PyDateTime> {
tzinfo: Option<&'py PyTzInfo>,
) -> PyResult<&'py PyDateTime> {
Self::from_timestamp_bound(py, timestamp, tzinfo.map(PyTzInfo::as_borrowed).as_deref())
.map(Bound::into_gil_ref)
}

/// Construct a `datetime` object from a POSIX timestamp
///
/// This is equivalent to `datetime.datetime.fromtimestamp`
pub fn from_timestamp_bound<'p>(
py: Python<'p>,
pub fn from_timestamp_bound<'py>(
py: Python<'py>,
timestamp: f64,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
) -> PyResult<Bound<'p, PyDateTime>> {
tzinfo: Option<&Bound<'py, PyTzInfo>>,
) -> PyResult<Bound<'py, PyDateTime>> {
let args = IntoPy::<Py<PyTuple>>::into_py((timestamp, tzinfo), py).into_bound(py);

// safety ensure API is loaded
Expand Down Expand Up @@ -579,14 +588,14 @@ impl PyTime {
note = "`PyTime::new` will be replaced by `PyTime::new_bound` in a future PyO3 version"
)
)]
pub fn new<'p>(
py: Python<'p>,
pub fn new<'py>(
py: Python<'py>,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&PyTzInfo>,
) -> PyResult<&'p PyTime> {
tzinfo: Option<&'py PyTzInfo>,
) -> PyResult<&'py PyTime> {
Self::new_bound(
py,
hour,
Expand All @@ -599,14 +608,14 @@ impl PyTime {
}

/// Creates a new `datetime.time` object.
pub fn new_bound<'p>(
py: Python<'p>,
pub fn new_bound<'py>(
py: Python<'py>,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
) -> PyResult<Bound<'p, PyTime>> {
tzinfo: Option<&Bound<'py, PyTzInfo>>,
) -> PyResult<Bound<'py, PyTime>> {
let api = ensure_datetime_api(py)?;
unsafe {
(api.Time_FromTime)(
Expand All @@ -630,15 +639,15 @@ impl PyTime {
note = "`PyTime::new_with_fold` will be replaced by `PyTime::new_bound_with_fold` in a future PyO3 version"
)
)]
pub fn new_with_fold<'p>(
py: Python<'p>,
pub fn new_with_fold<'py>(
py: Python<'py>,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&PyTzInfo>,
tzinfo: Option<&'py PyTzInfo>,
fold: bool,
) -> PyResult<&'p PyTime> {
) -> PyResult<&'py PyTime> {
Self::new_bound_with_fold(
py,
hour,
Expand All @@ -652,15 +661,15 @@ impl PyTime {
}

/// Alternate constructor that takes a `fold` argument. See [`PyDateTime::new_with_fold`].
pub fn new_bound_with_fold<'p>(
py: Python<'p>,
pub fn new_bound_with_fold<'py>(
py: Python<'py>,
hour: u8,
minute: u8,
second: u8,
microsecond: u32,
tzinfo: Option<&Bound<'_, PyTzInfo>>,
tzinfo: Option<&Bound<'py, PyTzInfo>>,
fold: bool,
) -> PyResult<Bound<'p, PyTime>> {
) -> PyResult<Bound<'py, PyTime>> {
let api = ensure_datetime_api(py)?;
unsafe {
(api.Time_FromTimeAndFold)(
Expand Down Expand Up @@ -762,9 +771,16 @@ pyobject_native_type!(
#checkfunction=PyTZInfo_Check
);

/// Equivalent to `datetime.timezone.utc`
/// Deprecated form of [`timezone_utc_bound`].
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`timezone_utc` will be replaced by `timezone_utc_bound` in a future PyO3 version"
)
)]
pub fn timezone_utc(py: Python<'_>) -> &PyTzInfo {
unsafe { &*(expect_datetime_api(py).TimeZone_UTC as *const PyTzInfo) }
timezone_utc_bound(py).into_gil_ref()
}

/// Equivalent to `datetime.timezone.utc`
Expand Down

0 comments on commit 10cb090

Please sign in to comment.