Skip to content

Create model::Date from OffsetDateTime, or another way of expanding supported date formats #581

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

Open
jesseditson opened this issue Mar 4, 2025 · 2 comments

Comments

@jesseditson
Copy link

Internally, DateTime is represented as

pub struct DateTime {
    inner: DateTimeImpl,
}

where

type DateTimeImpl = time::OffsetDateTime;

My implementation supports broader time parsing than liquid-rust, so I have a time::OffsetDateTime, and need a liquid::model::DateTime. Sadly, because inner is private, I have to serialize to a string and deserialize. I do this during batch imports and it's unnecessarily slow.

I understand that keeping the internal representation private is a reasonable decision given it gives you flexibility on representation later. I do think it might make sense to add an impl From<OffsetDateTime>, which sadly I cannot do from outside the crate given the visibility. This could obviously be feature flagged, and if you do change the inner implementation in the future, the implementation can just parse the OffsetDateTime into the new format, which is future proof and fairly low-cost.

today, the impl would be along the lines of

#[cfg(feature = "date-time-from-offset-date-time")]
impl From<time::OffsetDateTime> for DateTime {
    fn from(value: time::OffsetDateTime) -> Self {
        Self { inner: value }
    }
}
@jesseditson
Copy link
Author

Alternatively, since a lot of what my implementation does mirrors the internal parsing, the ability to add formats to USER_FORMATS in some way would solve my problem - however, this has all the same visibility issues (exposing format_description! as part of this crate's supported API), and probably is much harder to test around, so I doubt this would be a preferred way of expanding support.

@epage
Copy link
Member

epage commented Mar 4, 2025

add an impl From

This still puts time in the API. I'm also considering switching to jiffy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants