time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.11k stars 278 forks source link

Owned formats and representing formats as unit structs #546

Closed Kinrany closed 1 year ago

Kinrany commented 1 year ago

It is currently nontrivial to construct a unit struct that represents a format similar to the well-known ones.

This is convenient when it makes sense to implement other features for individual formats.

For example, impl MyFormat { pub fn deserialize() {} } makes it convenient to deserialize fields with #[serde(deserialize_with = "MyFormat::deserialize")].

One workaround that might work now is a struct MyFormat(OwnedFormatItem) with Deref. But constructing an OwnedFormatItem from a macro is also nontrivial.

Ideally we would have a way to compose unit struct format representations somehow.

Related: https://github.com/time-rs/time/issues/350

jhpratt commented 1 year ago

From what it sounds like, you're trying to use it with serde. That doesn't require a struct at all, and can be accomplished using the time::serde::format_description! macro.

This is unrelated to whether something is a struct, the struct's size, and constructing an OwnedFormatItem from a macro (which will be possible with the next release). I don't see how this is related to #350 in any way — that's an issue related to restructuring the crate to avoid duplicated code.

Kinrany commented 1 year ago

Related to #350 only in that that issue is about making formatting a general-purpose crate, and I expect that to solve this issue as a side effect.

jhpratt commented 1 year ago

Not at all. As I said, it is a strictly internal restructuring to avoid duplicating code. Even if the trait is made publicly implementable from a time-formatting crate, that doesn't mean there are any stability guarantees. On the contrary, there wouldn't be, just as there aren't any guarantees about its API currently.

Setting that aside, I still don't see why something has to be a unit struct. The performance gain would be essentially zero without writing the entire parser/formatter by hand, as is done for the well-known formats.

Kinrany commented 1 year ago

You're right, this is an X/Y problem. I don't want unit structs, I want to reuse formats.

With well-known formats it's super easy, but custom formats generated with time::macros::format_description have to be wrapped in lazy_static or a function. One has to type out the whole &'static [FormatItem<'static>] and either use a dependency or use the format as a function call.

Which is not terrible, but there's no reason in principle why custom formats can't look the same as well-known formats and can't be declared at the type level as one-liners.

serde::format_description is very good and indeed solves the problem for the case of serde. It would be nice if it could also accept an existing format, so that the same format string wouldn't have to be written twice.

Kinrany commented 1 year ago

Maybe this should have been a discussion 😄

jhpratt commented 1 year ago

custom formats generated with time::macros::format_description have to be wrapped in lazy_static or a function

What leads you to believe this? You can assign the value to a const directly. I have been careful to ensure that the values output are compatible with doing this. There is even one place where this is required to make tests compile: link. Having serde::format_description! support this is already implemented, just not released.

One has to type out the whole &'static [FormatItem<'static>] and either use a dependency or use the format as a function call.

'static can be elided in consts. So it only has to be &[FormatItem<'_>].

Kinrany commented 1 year ago

Oh, I never thought to try! Thank you 😄