chrono
chrono copied to clipboard
Add support for serialising and deserialising string timestamps
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
@djc Is my work so far on the right track? Would you like to request any changes to my approach?
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.
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).
@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 A lot of those lines are documentation and tests. I could potentially try to define a macro, but the tests and docs would remain.
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.
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).
There is already a similar feature in the container attributes: from and into.
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.
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.)
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.
I did see it. I'm fine with closing this PR or maybe substituting it with a documentation pointer to the serde_with crate.