Skip to content

Conversation

@ozpv
Copy link

@ozpv ozpv commented Feb 21, 2025

this pr will add chrono support to the crate and close issue #227

So far, I've created type aliases to the respective date/time types in chrono and time to make the implementation easier.

I found something wrong with this approach through. It works fine to have the drop-in replacement feature flag for new projects or projects that don't already depend on cookie in some way, but existing ones would have to implement support for the chrono feature flag to be able to take advantage of this.

If this isn't fine, I could create a seperate module with something like a ChronoCookie, which would just be a copy of Cookie using chrono instead. Then it would be possible to use Cookie::into_chrono or vice versa to convert between. The drawback of this approach is maintaining the conversions between time and chrono, and also requiring the time dependency as a chrono user, even if you never actually use it

Currently, however, it compiles with all features and all the parsing tests are passing for the chrono feature. I will get the rest of the tests working in the next couple of days and update the test script

A couple of updates outside of this feature would be to commit 3795f2e, where now abbreviated dates before 70 will be 21st century dates. This is because the browsers referenced in issue #162 have updated year normalization.

The other thing I updated was the tests for abbreviated dates, the day of the week was mismatched with the actual day, causing the chrono test to fail (it passes now). I'm unsure if it's the custom parsing function for short time dates that ignores this, or time itself, but that seems like it could be an issue

@Caellian
Copy link

Caellian commented Nov 14, 2025

Avoid re-parsing the format string at runtime:

    use chrono::format::Item;
    use chrono::format::{Item::*, Fixed::*, Numeric::*, Pad, Pad::Zero};
    
    // Following were produced by debug printing corresponding format strings using:
    // `chrono::format::StrftimeItems::new(FORMAT_STRING).parse().unwrap()`

    /// Format: %a, %d %b %Y %H:%M:%S GMT
    pub static FMT1: &[Item] = &[Fixed(ShortWeekdayName), Literal(","), Space(" "), Numeric(Day, Zero), Space(" "), Fixed(ShortMonthName), Space(" "), Numeric(Year, Zero), Space(" "), Numeric(Hour, Zero), Literal(":"), Numeric(Minute, Zero), Literal(":"), Numeric(Second, Zero), Space(" "), Literal("GMT")];
    /// Format: %A, %d-%b-%y %H:%M:%S GMT
    pub static FMT2: &[Item] = &[Fixed(LongWeekdayName), Literal(","), Space(" "), Numeric(Day, Zero), Literal("-"), Fixed(ShortMonthName), Literal("-"), Numeric(YearMod100, Zero), Space(" "), Numeric(Hour, Zero), Literal(":"), Numeric(Minute, Zero), Literal(":"), Numeric(Second, Zero), Space(" "), Literal("GMT")];
    /// Format: %a %b %_d %H:%M:%S %Y
    pub static FMT3: &[Item] = &[Fixed(ShortWeekdayName), Space(" "), Fixed(ShortMonthName), Space(" "), Numeric(Day, Space), Space(" "), Numeric(Hour, Zero), Literal(":"), Numeric(Minute, Zero), Literal(":"), Numeric(Second, Zero), Space(" "), Numeric(Year, Zero)];
    /// Format: %a, %d-%b-%Y %H:%M:%S GMT
    pub static FMT4: &[Item] = &[Fixed(ShortWeekdayName), Literal(","), Space(" "), Numeric(Day, Zero), Literal("-"), Fixed(ShortMonthName), Literal("-"), Numeric(Year, Zero), Space(" "), Numeric(Hour, Zero), Literal(":"), Numeric(Minute, Zero), Literal(":"), Numeric(Second, Zero), Space(" "), Literal("GMT")];

Also, the diff on this PR is extremely noisy. It's got a refactor, addition of chrono and another separate issue packed into two commits. Looking at the changes makes it very difficult to see what adding chrono actually did to the codebase - which is why it would be better those were in separate PRs.

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

Successfully merging this pull request may close these issues.

2 participants