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

Support deserializing from timestamps #125

Merged

Conversation

quodlibetor
Copy link
Contributor

This is almost identical to #124, but for serde 0.9. This is built on top of #122.

This is useful when using derive(Deserialize) on a struct that contains some sort of DateTime, but the foreign API returns timestamps instead of strs.

@quodlibetor quodlibetor force-pushed the serde-0.9-support-timestamps branch 2 times, most recently from ca205e9 to 9a39e97 Compare February 5, 2017 20:44
@quodlibetor
Copy link
Contributor Author

@lifthrasiir I'm not sure if this is something that you're interested in supporting in chrono, or if there are good reasons to avoid doing it.

All the web APIs that I interact with return integral timestamps in terms of seconds, but I think that "some" APIs give timestamps as milliseconds. I'm not sure if there's a good way of handling that generically.

@quodlibetor quodlibetor changed the title Serde 0.9 support deserializing from timestamps Support deserializing from timestamps Feb 11, 2017
@quodlibetor
Copy link
Contributor Author

I've re-worked this to support deserializing both serde and rustcserialize since that seems like something that is still desired.

Additionally, it no longer suffers from the ambiguity that made me uncomfortable when I first drafted this PR: users now need to explicitly opt-in to timestamp handling, and the type is pretty trivially expandable into deserializing from multiple different categories of timestamps.

That said, I'm not incredibly happy with this API, it feels like it's putting a larger-than-necessary burden on downstream crates: I don't feel like they should, necessarily, need to implement their custom types for the various Ts* types (TsSeconds, TsMillis, TsMicros, maybe TsNanos?) that would be needed with this scheme.

@quodlibetor
Copy link
Contributor Author

Also, the docs are currently pretty ugly, if this is a feature that's actually desired I'll clean them up and maybe clean up the module structure (right now everything just keeps getting dumped into the two datetime files.

@quodlibetor
Copy link
Contributor Author

It turns out that serde has an API that means that this can be supported with a non-horrible API, and no magic:

use chrono::datetime::serde::ts_seconds;
#[derive(Deserialize, Serialize)]
struct S {
    #[serde(with = "ts_seconds")]
    time: DateTime<UTC>
}

The with annotation can also be serialize_with = "ts_seconds::serialize" or deserialize_with = "ts_seconds::deserialize", included in the doc examples. This API doesn't require a wrapper struct, isn't implicitly seconds-based, and can be straightforwardly implemented for a variety of other serialization formats.

I'm currently fighting with travis, which does not seem to want to include the environment variables that I'm trying to specify in the build matrix to allow this to successfully compile on rust 1.13.0. This whole thing feels kind of hacky -- I want to include doc tests, but only on rust >= 1.15.0 because they rely on custom derive, which means that the .travis.yml is suddenly pretty gross.

This would also be significantly nicer with an optional serde_derive dev-dependency, but cargo doesn't support that yet (rust-lang/cargo#1596).

Additionally, trying to keep feature parity with rustc-serialize is basically impossible, RustcSerialize doesn't support (AFAICT) anything equivalent to serde's with helper. I'm currently including it because a recent commit (76df8e6) by @lifthrasiir suggests that feature parity for the two implementations is desired. Honestly this doesn't feel like feature parity (the Newtype API is pretty awful), rustc-serialize itself recommends that the community move to serde, and with the recent stabilization of custom derive it seems like the community is heading in that direction, so I'd prefer to keep the feature serde-only.

The code is still pretty ugly (I'm just pushing this version at this point because it's obviously better and I could use some feedback), but I'm happy to clean it up if there's interest in keeping this work.

Timestamps are defined in terms of UTC, so what this does is, if we encounter
an integer instead of a str, create a FixedOffset timestamp with an offset of
zero and create the timestamp from that.
This introduces a newtype around DateTime and NaiveDateTime that deserlization
is implemented for.

There are two advantages to this over the previous implementation:

* It is expandable to other timestamp representations (e.g. millisecond and
  microsecond timestamps)
* It works with RustcSerialize::Decodable. AFAICT Decodable will error if you
  try to call more than one of the `read_*` functions in the same `decode`
  invocation. This is slightly annoying compared to serde which just calls the
  correct `visit_*` function for whatever type the deserializer encounters.

On the whole I think that I prefer this to the previous implementation of
deserializing timestamps (even though I don't care about RustcSerialize in the
post-1.15 world) because it is much more explicit.

On the other hand, this feels like it's introducing a lot of types, and
possibly making downstream crates introduce a variety of different structs for
ser/de and translating into different struct types.
@quodlibetor quodlibetor force-pushed the serde-0.9-support-timestamps branch 2 times, most recently from d92457d to da24c1e Compare June 19, 2017 12:33
@quodlibetor
Copy link
Contributor Author

Okay I stole/modified the travis.sh script from serde and managed to get this building correctly (forgetting that we do, in fact, test on appveyor) using the same technique that serde uses -- test on stable/beta/nightly and just build on 1.13.

The code still needs to be cleaned up, and I would rather delete the rustc-serialize support than include it because the API is unpleasant and clutters up the code. But if you're interested in getting this merged before 0.4 @lifthrasiir I would really like it.

Maybe a high-level comment about the issues of including something like this? I wouldn't recommend doing a code review right now the code needs a bit of cleanup. It is a compact binary representation, which there used to be a comment saying we need.

@quodlibetor quodlibetor force-pushed the serde-0.9-support-timestamps branch 2 times, most recently from ee8f834 to 49f9648 Compare June 19, 2017 18:27
This is a significantly less horrible API than in the previous commit.
I think it's a terrible API, but AFAIK rustc-serialize doesn't support anything
like serde's `with` attribute.

I think it would be better to just not include this API at all and require that
people who want to use this move to serde, which is the recommended rust
encoding/decoding library.
@lifthrasiir
Copy link
Contributor

I'm not yet sure how this PR will react with #161, but the commits are very solid and I'll merge without further ado. Thank you!

@lifthrasiir lifthrasiir merged commit 4e7b840 into chronotope:master Jun 20, 2017
@quodlibetor
Copy link
Contributor Author

@lifthrasiir would you like me to put up a PR for #161? I would guess that I could have it up in less than 10 hours if you haven't started work/got time to work on it, especially if that's the main thing that's blocking 0.4.

@lifthrasiir
Copy link
Contributor

@quodlibetor Thank you for the consideration but I've done the work :-)

@quodlibetor quodlibetor deleted the serde-0.9-support-timestamps branch June 22, 2017 01:38
@pickfire
Copy link

Should we add support for Option<Datetime<_>> as well?

@quodlibetor
Copy link
Contributor Author

@pickfire you can deserialize from options with a trivial wrapper function:

use chrono;
use chrono::serde::ts_seconds::deserialize as from_ts_seconds;
use serde::de::Deserializer;

pub fn option_dt<'de, D>(de: D) -> Result<Option<chrono::DateTime<chrono::Utc>>, D::Error>
where
    D: Deserializer<'de>,
{
    from_ts_seconds(de).map(Some)
}

#[derive(Deserialize)
struct MyThing {
    #[serde(deserialize_with = "option_dt")]
    my_date: Option<DateTime<Utc>>,
}

@pickfire
Copy link

pickfire commented May 11, 2018

@quodlibetor Oh, thanks a lot. I didn't though that it can be done like that.

@quodlibetor
Copy link
Contributor Author

Yeah it's not super obvious, I learned that it was possible from something somewhere. I think it's in the serde docs in one of the examples? If not it should be.

@pickfire
Copy link

There is one in https://serde.rs/custom-date-format.html but not this example.

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.

None yet

3 participants