Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Owned formats and representing formats as unit structs #546

Closed
Kinrany opened this issue Jan 29, 2023 · 7 comments
Closed

Owned formats and representing formats as unit structs #546

Kinrany opened this issue Jan 29, 2023 · 7 comments
Labels
C-question Category: seeking clarification on a topic

Comments

@Kinrany
Copy link
Contributor

Kinrany commented Jan 29, 2023

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: #350

@jhpratt
Copy link
Member

jhpratt commented Jan 29, 2023

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
Copy link
Contributor Author

Kinrany commented Jan 30, 2023

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
Copy link
Member

jhpratt commented Jan 30, 2023

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.

@jhpratt jhpratt added the C-needs-details Category: more details are needed to assess the situation label Feb 1, 2023
@Kinrany
Copy link
Contributor Author

Kinrany commented Feb 2, 2023

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
Copy link
Contributor Author

Kinrany commented Feb 2, 2023

Maybe this should have been a discussion 😄

@jhpratt
Copy link
Member

jhpratt commented Feb 2, 2023

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
Copy link
Contributor Author

Kinrany commented Feb 2, 2023

Oh, I never thought to try! Thank you 😄

@jhpratt jhpratt closed this as completed Feb 2, 2023
@jhpratt jhpratt added C-question Category: seeking clarification on a topic and removed C-needs-details Category: more details are needed to assess the situation labels Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question Category: seeking clarification on a topic
Projects
None yet
Development

No branches or pull requests

2 participants