Add jiff into/from python convertions

[bschoenmaeckers]

closes #4510

[bschoenmaeckers]

Jiff requires rust 1.70 or newer, but pyo3's minimum version is 1.63.0. Is it acceptable to increase our msrv or can we make this conditional somehow?

[Icxolu]

I don't think we can bump MSRV, but having an off-by-default feature, which requires a newer version is probably ok (if we document that). I think this therefore requires changes in the testing workflows, because it can't be part of full but still should run on everything except the MSRV workflow.

[bschoenmaeckers]

Fixed the CI and all other TODO's so it is ready for review

[Icxolu]

Thank you! This looks good to me, just some minor comments.

[Icxolu]

Could you add a comment here on why this is unreachable.

[BurntSushi]

Here's my attempt:

// This case isn't reachable because if it were, it
// would imply that the `Zoned` value has a civil
// time that never appears on a clock in this time
// zone. Jiff's API prevents this from happening,
// so this case is unreachable.

An alternative for discovering whether a datetime is in a fold or not might be to use the new TimeZone::preceding and TimeZone::following APIs. It's likely faster but more difficult to get right. The way it's done here is probably the clearest way to do it given Jiff's current API.

[Icxolu]

Sounds good to me 👍, thank you for the suggestion!

[bschoenmaeckers]

An alternative for discovering whether a datetime is in a fold or not might be to use the new TimeZone::preceding and TimeZone::following APIs.

Switched to the TimeZone iter API & added a proptest to verify. 👍

[bschoenmaeckers]

It might be a nice api addition to jiff if one could get the start timestamp of the current offset transition.

I currently do it using the following snippet but it could probably be done cleaner with some internal api?

            let prev = zoned.time_zone().preceding(zoned.timestamp()).next()?;
            let start_of_current_offset = zoned
                .time_zone()
                .following(prev.timestamp())
                .next()?
                .timestamp();

[BurntSushi]

I think that's actually not quite right. Try this:

use jiff::tz::{TimeZone, TimeZoneTransition};

fn main() -> anyhow::Result<()> {
    let tz = TimeZone::system();
    let zdt = jiff::Zoned::now();
    let cur = current_transition(&tz, zdt.timestamp()).unwrap();
    dbg!(cur.timestamp().to_zoned(tz));
    Ok(())
}

fn current_transition(
    tz: &TimeZone,
    ts: jiff::Timestamp,
) -> Option<TimeZoneTransition> {
    let prev = tz.preceding(ts).next()?;
    let next = tz.following(ts).next()?;
    if next.timestamp() == ts {
        Some(next)
    } else {
        Some(prev)
    }
}

Jiff actually doesn't have any internal APIs that would make this easier. It might be worth adding a new routine for this, but I'd like to see how common the need is for it first.

[bschoenmaeckers]

When ts is exactly on a timezone transition both preceding and following will not contain the current transition. preceding will contain all transitions strictly earlier and following will contain all transitions strictly after the current one. So it is impossible for the case 2020-10-25T01:00:00+00:00[Europe/London] to get the current transition without going back one transition and going forward again.

zoned: 2020-10-25T01:00:00+00:00[Europe/London]
zoned.timestamp(): 2020-10-25T01:00:00Z
next.timestamp(): 2021-03-28T01:00:00Z
prev.timestamp(): 2020-03-29T01:00:00Z

[BurntSushi]

Yeah my code does do that! But I did indeed have a bug in it. Try this instead:

fn current_transition(
    tz: &TimeZone,
    ts: Timestamp,
) -> Option<TimeZoneTransition> {
    let prev = tz.preceding(ts).next()?;
    let next = tz.following(prev.timestamp()).next()?;
    if next.timestamp() == ts {
        Some(next)
    } else {
        Some(prev)
    }
}

That should work. In your original code, you go back and forward unconditionally, which only works in the case where the original timestamp is precisely at the boundary. But the above snippet only uses next (after going back) if the given timestamp is at the boundary, otherwise, the immediately previous transition is the correct one.

Your code is correct if you know the timestamp given is always at a boundary.

[bschoenmaeckers]

Thanks you are right and thanks for pointing this out. Code looks way cleaner this way.

[BurntSushi]

I would suggest ensuring that the feature is named jiff-01, either by introducing a new feature or renaming the package. For example, this is what rust-postgres does.

This ensures there is a paved path to supporting multiple versions of Jiff when a semver incompatible release comes out without making a semver incompatible release of pyo3.

Arguably, this ought to be done for other dependencies, but maybe they don't do incompatible releases often. For Jiff, I'd like get 1.0 out this summer, and then it should sit there for a long while.

[Icxolu]

That's a good idea (especially if we know there will be a major version bump soon-ish), thanks for bringing that up. Renaming the package looks like a good option to me.

[Icxolu]

We should also add this to the feature docs in the guide, and mention the different MSRV for this feature there.

[BurntSushi]

It might be worth waiting on this a little bit. I'm planning to get a jiff 0.2 release out in the next month or so. So if this gets merged for jiff 0.1, there will likely be some upcoming churn pretty soon.

My plan is to get jiff 0.2 out with some breaking changes we've discovered. And if that works out okay for the next six months or so, then I want to get Jiff 1.0 out in the summer and I plan to commit to that longer term (~years, or ideally, indefinitely).

[Icxolu]

It might be worth waiting on this a little bit. I'm planning to get a jiff 0.2 release out in the next month or so.

I'd be fine with that. (Something like ">= 0.1, < 0.3" could also be an option, but that depends on the changes)

[davidhewitt]

Thanks, this is looking good to me (I didn't manage to read the meat of the conversions in much depth, it looks like they have already been reviewed, thanks @Icxolu).

I'm ok with either merging this for jiff 0.1 or waiting until jiff 0.2 as you prefer @bschoenmaeckers.

Just a few small thoughts.

[davidhewitt]

Maybe this is better named as datetime_abi3? It's not really an ABI here but it is code specific to how we deal with abi3.

[davidhewitt]

The clippy jobs are already quite slow. Rather than adding additional jobs here, please can you modify _get_feature_sets() to be rust version aware and only set the jiff feature if the compiler is new enough?

[davidhewitt]

You might need to cfg this doctest as not windows, IIRC windows needs a tzdata package but it's quite a faff to bother requiring it.

[bschoenmaeckers]

I'm ok with either merging this for jiff 0.1 or waiting until jiff 0.2 as you prefer @bschoenmaeckers.

I'm fine by merging it for 0.1, adding support for 0.2 should be very easy when the time comes.

[Icxolu]

Thanks again for implementing! Let's get this merged now 🚀

[Icxolu]

Looks like the feature combinations for CI are not quite right yet.

[bschoenmaeckers]

Looks like the feature combinations for CI are not quite right yet.

Please try again