chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Add support for serialising and deserialising string timestamps

Open sinking-point opened this issue 2 years ago • 12 comments

This PR aims to address https://github.com/chronotope/chrono/issues/196#issuecomment-1549430697

  • [x] Implement ts_seconds_string (in DateTime)
  • [x] Implement ts_milliseconds_string
  • [x] Implement ts_microseconds_string
  • [x] Implement ts_nanoseconds_string
  • [x] Add optionals (e.g. ts_seconds_string_option)
  • [x] Support NaiveDateTime as well

sinking-point avatar May 16 '23 14:05 sinking-point

@djc Is my work so far on the right track? Would you like to request any changes to my approach?

sinking-point avatar May 16 '23 14:05 sinking-point

Thanks for the feedback @djc . There might be some more messy commits as I continue working on this, but I'll ensure to clean them up when I'm done.

sinking-point avatar May 17 '23 08:05 sinking-point

I've changed this to draft status for now, please switch it to ready once you think it's ready (or just comment if you want feedback on something sooner).

djc avatar May 17 '23 12:05 djc

@sinking-point I have only glanced at the code, apologies for that. The amount of lines needed for (de)serializing is massive! Is there a way to collapse that into something manageable with a macro?

pitdicker avatar May 18 '23 17:05 pitdicker

@pitdicker A lot of those lines are documentation and tests. I could potentially try to define a macro, but the tests and docs would remain.

sinking-point avatar May 18 '23 17:05 sinking-point

The reason there's so much repetition is that there are 4 dimensions: int/string, normal/optional, DateTime/NaiveDateTime, nano/micro/milli/seconds. I agree that this isn't very DRY and it would be good to find a DRYer way of doing it.

sinking-point avatar May 18 '23 17:05 sinking-point

TBH this is difficult without extending serde's field attributes. It would be nice if there was a via attribute that takes another type (e.g. TimestampNanoseconds). Serde would deserialise into that first, then into() it to the target type (e.g. DateTime).

sinking-point avatar May 18 '23 18:05 sinking-point

There is already a similar feature in the container attributes: from and into.

sinking-point avatar May 18 '23 18:05 sinking-point

Here's the usage I'm envisioning:

use chrono::{TimeZone, DateTime, Utc, NaiveDate};
use serde_derive::{Deserialize, Serialize};
use chrono::serde::{Timestamp, Nanoseconds};
#[derive(Deserialize, Serialize)]
struct S {
    #[serde(via = "Timestamp<Nanoseconds, i64>")] // could equally be Timestamp<Seconds, String>
    time: DateTime<Utc> // could equally be NaiveDateTime - both would implement From<Timestamp>
}

Maybe Timestamp could even take a timezone parameter as well, which would allow automatic conversion to the timezone of the DateTime.

To reiterate, this via attribute doesn't exist yet. We'd need to get it into serde for this idea to be possible.

sinking-point avatar May 18 '23 18:05 sinking-point

Given that the with attribute just points to deserialize() and serialize() functions in a module, there's a a lot of freedom with what we can do within those functions. So it feels like we should be able to do your Timestamp<Nanoseconds, i64> thing with some minimal boilerplate wrapping?

(Or macros, which would probably make this easier if also a little uglier. BTW, I don't see why we couldn't have the macros generate the tests, too.)

djc avatar May 19 '23 11:05 djc

We could, but if we still want to have docs and tests for each variant then it wouldn't reduce the line count by all that much.

In my opinion, tests should be concrete. I feel that tests are best when they're a self-contained, readable specification of what a module should do. Also, one function of the tests would be to test the assumptions made by the programmer in defining the macros. However, if the tests are also defined by a macro then they will be influenced by those same assumptions, which might cancel out.

Have you seen my comment on the issue? It seems that implementing my proposal would almost just be reimplementing part of the serde_with crate.

sinking-point avatar May 19 '23 12:05 sinking-point

I did see it. I'm fine with closing this PR or maybe substituting it with a documentation pointer to the serde_with crate.

djc avatar May 19 '23 13:05 djc