BurntSushi / jiff

A date-time library for Rust that encourages you to jump into the pit of success.
The Unlicense
1.64k stars 26 forks source link

serde support: `TimeZone` or `Offset` #89

Closed onkoe closed 2 weeks ago

onkoe commented 1 month ago

Hey there! Thanks for this incredible new crate!

I'm using it in a new project, but I'm having difficulties due to the TimeZone and Offset types lacking serde implementations. I can understand why a TimeZone might not have it (the design document was very convincing).

However, is it intentional that Offset doesn't have one? This seems like it'd just serialize that internal integer. (The define_ranged! machine does look mildly complicated here, of course).

Please let me know if I'm missing anything, or there's something else that might be better for serializing a user's timezone.

Once again, thanks for the new contribution to the ecosystem! :)

BurntSushi commented 1 month ago

Can you say more about why you're serializing these types separate from the datetime types?

onkoe commented 1 month ago

I'm building a Discord bot for friends and want to keep things simple by tying them with their time zones.

Since I'm already using jiff for its more commonplace datetime-focused types, it works out nicely since I don't need to write a bunch of conversions from some other timezone type!

Please let me know if that helps enough. I'm happy to give code examples! :)

BurntSushi commented 1 month ago

Right, so you should be able to use Zoned for that. That is, it will include the time zone and offset when serializing it. Most datetime libraries don't do this, so you might be used to serializing the time zone separately.

But reading closer, it looks like you want to associate the time zone itself with the user outside the context of a datetime. Which I think makes sense. Can you say though why you want to serialize an offset instead of just a time zone?

In terms of technical implementation, there isn't any problem with adding a serde impl for Offset. Whether to add it or not is really about API design and being careful about avoiding encouraging folks to do the wrong thing. So for the Offset specifically, I would need to think about it.

But for TimeZone, that's harder, because there just simply may not be a compact serializable representation of it.

Instead, I would ask you to consider storing a IANA time zone identifier for each of your friends. It might be missing if you don't know it. Then you would just call TimeZone::get on the stored string when you need the actual time zone. Those calls are cached. And you can get the identifier from a time zone with TimeZone::iana_name if one exists.

Does that help?

onkoe commented 1 month ago

Great, thank you!

My idea for using an Offset over TimeZone is that a TimeZone has attributes that change, but the bot is built around one central pillar: it must store the "availability" of people, then compare the availabilities of all users when a user sends a command to the bot.

Currently, I do this by storing a Vec<Span> alongside the user, but that list can sometimes be empty when the user is always available. Here's how that looks:

/// A representation of someone, with some of their details important to
/// knowing when to ping.
#[non_exhaustive]
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct Person {
    user_id: UserId,
    timezone: Offset,
    /// A list of timespans where they're unavailable (gone).
    gone: Vec<Unavailability>,
}

impl Person {
    /// Given a `UserId`, creates a new person with "default" availability
    /// and timezone (i.e. both blank).
    pub fn new(user_id: UserId) -> Self {
        Self {
            user_id,
            timezone: Offset::UTC,
            gone: Vec::new(),
        }
    }
}

/// A span of time where someone isn't able to play.
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct Unavailability {
    span: Span,
}

For this code to work, I need the user's timezone to be stored independently of their availability. Otherwise, I can't convert their spans if they change their timezone. Using Offset here isn't too important, but I know it's easier to serialize a simple offset than encoding the complexities of a timezone.

It's also less fallible, as I don't need to 'convert' the list of Spans to the timezone each time I try to use them. (If I didn't do that with a TimeZone, the spans could be wrong from moment-to-moment.)

Timezone Identifiers

I wouldn't mind storing a timezone identifier, so long as I can easily find them. Can I get it from a given Offset? A Zoned? Or even a TimeZone itself? If not, I'll likely have to make my own type, which is something I really want to avoid! 😄

Would it make sense to store a string? Would it be safe? Thank you!

BurntSushi commented 1 month ago

My take is that you should 100% be storing an IANA time zone identifier. And yes, it should be stored as a string. And no, you cannot get an IANA time zone identifier from an offset because there may be many IANA time zone identifiers for the same offset. Instead, it's the reverse: you get an offset from a time zone and either a Timestamp (instant in time) or a civil::DateTime (an end user's local time). How you get an IANA time zone identifier from an end user is I guess your job. How do you get their offset? You might need to ask them.

Using an IANA time zone identifier actually seems critical to your use case here, to the point that storing an offset would be completely wrong. For example, for where I live, my IANA time zone identifier is always America/New_York. The only way that would change is if I physically moved. But my offset changes twice per year: it goes from -05 to -04 in the late winter (this year it was March 10, 2024) and -04 to -05 in mid-autumn (this year it will be November 3, 2024). This is true in many locations in the world and is known as daylight saving time. If you only capture the offset for each user at the present time, all of your calculations will be wrong once they enter or leave daylight saving time. But if you use an IANA time zone identifier and the Zoned data type, then you literally don't need to worry about this at all. Jiff will handle everything and should actually make it impossible for you to get it wrong.

onkoe commented 1 month ago

Storing an offset is definitely wrong to some degree! To address the issues you mentioned, I'll probably make a serde-impl'd type that holds a String and interfaces with jiff to check the Spans when they're needed.

Nonetheless, I'd love to see some form of serialization. So, assuming you'd want to serialize in IANA form, that leaves just two questions:

Answering these questions would solve this issue. However, for my simple project, using String timezone identifiers and Timestamp in the Span list would be an adequate solution. Thank you for your assistance!

BurntSushi commented 1 month ago

Can jiff currently get an IANA identifier from each 'variant' of TimeZone?

No, it can't. That's why TimeZone::iana_name returns an option. It is fundamentally impossible to always have an IANA identifier. But that is the happy path. A missing TZ identifier is a generally a sub-optimal situation.

As for exposing the inner TZif, it is in theory possible, but it's probably a bad idea. I'm more likely to go the route outlined in https://github.com/BurntSushi/jiff/issues/30. But making that "easy" via a serde impl is almost certainly the wrong thing to do.

onkoe commented 1 month ago

I tend to agree on the serde part - deciding for users seems potentially harmful.

The plan in #30 sounds pretty good! Please let me know if you see any movement there. I'll watch the issue in the meantime.

BurntSushi commented 2 weeks ago

Going to close this out in favor of #30. (Although, from my perspective, it does seem like just an IANA tz identifier is sufficient here for the OP's use case.)