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

Offer an option to serialize/deserialize to/from RFC 2822 #1292

Open
djc opened this issue Sep 15, 2023 · 7 comments · May be fixed by #1477
Open

Offer an option to serialize/deserialize to/from RFC 2822 #1292

djc opened this issue Sep 15, 2023 · 7 comments · May be fixed by #1477
Labels

Comments

@djc
Copy link
Contributor

djc commented Sep 15, 2023

For work, I'm currently implementing RSS feeds. These use RFC 2822 date/times for the pubDate field. It would be nice if there was a serde::rfc2822 module that I could use similar to the other options we currently offer there.

@chrisranderson
Copy link

I'd like to work on this one - seems like it could be a good first issue for someone new to Rust/chrono. I've poked around for a while, and I think I have a good direction for implementation and tests based on other examples in serde.rs.

@chrisranderson
Copy link

Here's my first shot at it. Can someone take a look and see if I'm on the right track? chrisranderson#1

@pitdicker
Copy link
Collaborator

Great, thank you for working on this!

I had an experiment to replace all the serde boilerplate with a macro. https://github.com/pitdicker/chrono/tree/serde_experiment

With that macro adding a new serialization format becomes just ca. 25 lines and documentation:

serde_module! {
    type: DateTime<Utc>,
    module: ts_nanoseconds,
    module_opt: ts_nanoseconds_option,
    doc: r##"
# Errors

An `i64` with nanosecond precision can span a range of ~584 years. Serializing a `DateTime` beyond
that range returns an error.

The dates that can be represented as nanoseconds are between 1677-09-21T00:12:44.0 and
2262-04-11T23:47:16.854775804.

# Example:

```rust
# use chrono::{DateTime, Utc, NaiveDate};
# use serde_derive::{Deserialize, Serialize};
use chrono::serde::ts_nanoseconds;
#[derive(Deserialize, Serialize)]
struct S {
    #[serde(with = "ts_nanoseconds")]
    time: DateTime<Utc>
}

let time = NaiveDate::from_ymd_opt(2018, 5, 17)
    .unwrap()
    .and_hms_nano_opt(02, 04, 59, 918355733)
    .unwrap()
    .and_utc();
let my_s = S {
    time: time.clone(),
};

let as_string = serde_json::to_string(&my_s)?;
assert_eq!(as_string, r#"{"time":1526522699918355733}"#);
let my_s: S = serde_json::from_str(&as_string)?;
assert_eq!(my_s.time, time);
# Ok::<(), serde_json::Error>(())
```"##,
    serialize_i64: |dt| DateTime::timestamp_nanos_opt(dt).ok_or("value out of range for a timestamp with nanosecond precision"),
    deserialize_i64: NanoSecondsTimestampVisitor,
    expecting: "a unix timestamp in nanoseconds",
    visit_i64(i64): |value: i64| serde_from(
        crate::Utc.timestamp_opt(
            value.div_euclid(1_000_000_000),
            (value.rem_euclid(1_000_000_000)) as u32,
        ),
        &value,
    ),
    visit_u64(u64): |value: u64| serde_from(
        Utc.timestamp_opt(
            (value / 1_000_000_000) as i64,
            (value % 1_000_000_000) as u32,
        ),
        &value,
    ),
}

I hoped this could be a good alternative to the 3000 lines #1067 was adding for 8(?) extra serialization formats.

@djc Do you have a preference? Would a macro to abstract all the boilerplate away be acceptable? Or should we just add support for the RFC 2822 format first?

@pitdicker
Copy link
Collaborator

Here's my first shot at it. Can someone take a look and see if I'm on the right track? chrisranderson#1

I could comment on a few nits, but it looks pretty much like it should.
I believe all other deserialize implementations deserialize to DateTime<FixedOffset>? We should do so here to. DateTime<Utc> discards the serialized offset info.
And all other (de)serializers support an Option variant.

@djc
Copy link
Contributor Author

djc commented Jan 31, 2024

I guess I'm open to using macros for this if there is no good way to do this without macros. However, if we do so I'd prefer to make the DSL the macro accepts more like regular Rust code -- IMO this setup makes it pretty unclear how this is turned into the output.

@chrisranderson
Copy link

I see two options - one, I submit a PR nowish with the adjustments @pitdicker mentioned; two, I wait until serialization formats are refactored/settle a bit and submit a PR following the new pattern. I'll wait for now.

@pitdicker
Copy link
Collaborator

Sorry for derailing. I've cleaned up the idea to use a macro and opened #1396. That may take a while to get merged though, if at all.

Feel free to submit a PR before that if you want to, it will still be appreciated.

@pitdicker pitdicker linked a pull request Mar 1, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants