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

Add Serialize/Deserialize for Duration #117

Open
botev opened this issue Jan 14, 2017 · 34 comments
Open

Add Serialize/Deserialize for Duration #117

botev opened this issue Jan 14, 2017 · 34 comments
Labels

Comments

@botev
Copy link

botev commented Jan 14, 2017

For features serde and rustc-serialize

@stusmall
Copy link

+1 I could really use implementation of Serialize/Deserialize for a project I'm working on.

@JoeyAcc
Copy link

JoeyAcc commented Jan 24, 2017

I wholeheartedly agree. In fact, i'd like to see serialization support for all the core types: Date, Time, DateTime, UTC, Local etc.

At the moment I need to define newtypes in my own code that basically do nothing except handle Serde support. It would be absolutely wonderful to just scrap all that serialization code, as it's rather boilerplate-heavy, and thus helps turn otherwise-nice code into a "Can't see the forest for the trees" kind of deal.

If this issue can be taken care of by adding derives to the relevant structs, I'd be willing to create a pull request with the changes.
If not, there will be too many (potentially gnarly) time-related details to quickly get the job done, and thus I'd rather leave it to someone who already has the necessary knowledge about de/serializing times and dates.
Does anyone know whether a couple of derives would be sufficient here?

@Geobert
Copy link

Geobert commented May 14, 2017

Hi @JoeyAcc,
I'm learning Rust at the moment, what are the newtypes you talked about?

@dylanede
Copy link

dylanede commented Jun 1, 2017

I would also really appreciate support for (de)serialising Duration. But it seems that that type comes from the time crate, which is deprecated. What's the situation regarding the dependency on time, @lifthrasiir?

@JoeyAcc
Copy link

JoeyAcc commented Jun 26, 2017

@Geobert I only now see this response, sorry about that.

A newtype is a wrapper type around one other type, for details have a look at this.

How that plays out in this context is that you write such a newtype N for e.g. Duration, and then implement Serialize and Deserialize for N rather than Duration (which is not possible as both it and Serde's traits are defined outside your own crate).

By manipulating exactly how the internal Duration is de/serialized (which is done by manually implementing Serialize and Deserialize) it's possible to get around the whole "I can't serialize this!" issue, at the cost of a fair amount of boilerplate.

That is also where my request to implement serialization directly for all chrono types comes from.

@quodlibetor
Copy link
Contributor

This would need to be implemented in rust-lang-deprecated/time to be done correctly. I believe that @lifthrasiir is working on writing a new Duration for chrono itself, which will make it possible for us to do this trivially.

For now you could write your own deserializer, see my comment here for an example of how.

If you feel like doing a particularly good job I would take a PR that implements that in chrono, keeping in mind that we're getting rid of the Duration before 1.0.

@JoeyAcc
Copy link

JoeyAcc commented Jun 27, 2017

If I understand you correctly, by "getting rid of the Duration" you mean the impl from rust-lang-deprecated/time?
In other words, from an API perspective it wouldn't be removed so much as completely replaced by another type that has the same Duration name?

@quodlibetor
Copy link
Contributor

Yes, mostly. A new Duration will be created entirely within Chrono. We will probably continue to export the OldDuration struct for one or two releases before it is deleted and only the chrono-internal Duration exists, but I'm not sure what the plan actually is.

@lifthrasiir
Copy link
Contributor

lifthrasiir commented Jun 27, 2017

Chrono (at least for the 0.4.x series) will continue to support three "duration-like" types:

  • std::time::Duration for the standard library compatibility
  • time::Duration for the backward compatibility (this is still mandatory, but the plan is to make the time dependency optional with the introduction of TimeSpan below)
  • chrono::TimeSpan which will be the new signed duration-like type, planned for 0.4.1

The new TimeSpan type will be used for the "natural" date and time computation then. I've almost finished the design of TimeSpan and am filling the gaps in the internals.

@JoeyAcc
Copy link

JoeyAcc commented Jun 28, 2017

Thank you both for the explanations.
I have one more question: Will the new TimeSpan type be backed by a similar second + nanosecond field construction as Duration or will it use the new u218 or i128 types that are in the process of being stabilized?

@lifthrasiir
Copy link
Contributor

@JoeyAcc It will be essentially the same format as std::time::duration except for being signed. There is no reason to use 128-bit integers for TimeSpan because it is simply too large to be useful---even 64-bit integers in the nanosecond precision will suffice for more than 500 years. The main reason to split the seconds (64 bits) and nanoseconds (32 bits) is that it is more efficient for most use cases, as many APIs and data structures do not directly count the number of nanoseconds.

@JoeyAcc
Copy link

JoeyAcc commented Jun 29, 2017

I see, that makes sense. I was not aware of those restrictions on the design space, so I kept wondering why have the sec/nanosec split at all. At first I thought it was due to the value range of u64/i64. But as you already remarked, The range is sufficient for a couple of hundred more years*.
Thank you for explaining this to me.

*This is assuming the hardware stays at nanosecond precision, which may not hold. I myself wouldn't mind moving as far as we can in the direction of the Planck time unit, for example.

@lifthrasiir
Copy link
Contributor

lifthrasiir commented Jun 29, 2017

This is assuming the hardware stays at nanosecond precision, which may not hold. I myself wouldn't mind moving as far as we can in the direction of the Planck time unit, for example.

In that case we can always make TimeSpan::with_attos(secs, nanos, attos) and extend the TimeSpan to have another u32 field :-) (This method of extension can be seen in, for example, TAI64.)

@Procrat
Copy link

Procrat commented Jun 2, 2018

I'm not entirely following the history of this issue, but I noticed that the dependency on the time crate is optional now and that the implementation of Duration is ported to this crate. Does that mean it is now actually possible to solve this issue?

@matthiasbeyer
Copy link

+1 I could really use implementation of Serialize/Deserialize for a project I'm working on.

Same here.

@quodlibetor
Copy link
Contributor

Yeah now that we are working on completely owning time I agree with getting an implementation for this in.

My inclination is to emit something like SECS[.FRAC] as the default, but maybe something like ISO8601 periods would make it more obvious that this is intended to be a duration? We can always add serde helpers to make the non-default case reasonably pleasant. Another option would be something like python's HH:MM:SS, which is a bit more pleasant to read at the expense of being kinda misleading.

@fenhl
Copy link

fenhl commented Mar 1, 2019

I like the “total number of seconds” serialization. Nice and simple, unlike ISO 8601 periods.

@garro95
Copy link

garro95 commented Apr 4, 2019

I would stay on the standard as a default, and use the with attribute for different kinds of de/serializations

@jean-airoldie
Copy link
Contributor

Support for a human readable format would be helpfull for config files. Something akin to humantime.

@Stargateur
Copy link

the time crates has now a serde features too, chrono could just update time dependencies.

@giacomocariello
Copy link

+1

@Exr0n
Copy link

Exr0n commented May 4, 2020

@quodlibetor I'm a bit of a rust newbie, but I'd like to give a shot at implementing the traits. I saw your linked comment, but I'm not sure where to start. Can you give me some quick pointers?

@GopherJ
Copy link

GopherJ commented Nov 16, 2020

+1

some infos that I found:

@mosesonline
Copy link

+1

1 similar comment
@leontoeides
Copy link

+1

@timvisee
Copy link

timvisee commented Jun 4, 2021

The current Duration type seems to be: https://docs.rs/chrono/0.4.19/chrono/struct.Duration.html

Is there any reason for it not supporting Serialize/Deserialize yet other than development time? Are there implementation details I should know about? Because I'd be happy to take this.

Edit: looking into this now.

@timvisee
Copy link

timvisee commented Jun 4, 2021

I did some investigation, here are my findings:

  • chrono still uses the Duration type provided by the time crate.
  • chrono currently uses time 0.1, which doesn't provide serde support for Duration.
  • time 0.2 does support serde for Duration, behind the serde feature.

So to fix this, time must be updated to 0.2 first.

Then, the serde feature in time must be enabled. This is tricky, because chrono provides the serde feature and crate. You can't specify a feature with the same name as one of your crates in the current Rust stable version due to name collisions. This can be fixed with feature namespaces.

In other words, we'd like to specify the following feature in Cargo.toml but feature namespaces (notice dep:) are yet to be stabilized:

[features]
# --- snip ---
serde = ["dep:serde", "time/serde"]

There are two alternatives for this to choose before stabilization. The first is to enable serde by default for time. The second alternative is to provide a feature such as time-serde = ["time/serde"]. We probably don't want either of these alternatives for various reasons.


So here are the steps I suggest:

TODO:

@TimYorke
Copy link

Unfortunately this needs revisiting - #553 has been closed

@djc
Copy link
Member

djc commented Mar 28, 2022

Similarly to the additional serde formats we support for things like DateTime, I'd be happy to review a PR that implements custom serializer/deserializer modules for the Duration type. Given how they're implement (using serde's with annotations), I don't think these would run into coherence problems.

@nyovaya
Copy link

nyovaya commented Jun 3, 2022

I wholeheartedly agree. In fact, i'd like to see serialization support for all the core types: Date, Time, DateTime, UTC, Local etc.

At the moment I need to define newtypes in my own code that basically do nothing except handle Serde support. It would be absolutely wonderful to just scrap all that serialization code, as it's rather boilerplate-heavy, and thus helps turn otherwise-nice code into a "Can't see the forest for the trees" kind of deal.

If this issue can be taken care of by adding derives to the relevant structs, I'd be willing to create a pull request with the changes. If not, there will be too many (potentially gnarly) time-related details to quickly get the job done, and thus I'd rather leave it to someone who already has the necessary knowledge about de/serializing times and dates. Does anyone know whether a couple of derives would be sufficient here?

Which types do even support Serialization atm?

@Odonno
Copy link

Odonno commented Jun 9, 2024

Any update on this?

@djc
Copy link
Member

djc commented Jun 9, 2024

I think we're ready to do this -- anyone want to send a PR?

@JustusFluegel
Copy link

JustusFluegel commented Jul 7, 2024

I also would love if (DateTime<T>, TimeDelta) would implement Serialize and Deserialize such that it is compatible with iso8601 (as intervals, meaning either startdate+duration, startdate+enddate or duration+enddate, and default to startdate+duration for the Serialize impl or something)

@wcarmon
Copy link

wcarmon commented Aug 30, 2024

Maybe something like this:

I'm not sure where to copy and paste this ...

pub mod timedelta_milliseconds {
    use chrono::TimeDelta;
    use serde::{Deserialize, Deserializer, Serializer};

    pub fn serialize<S>(duration: &TimeDelta, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        serializer.serialize_i64(duration.num_milliseconds())
    }

    pub fn deserialize<'de, D>(deserializer: D) -> Result<TimeDelta, D::Error>
    where
        D: Deserializer<'de>,
    {
        Ok(TimeDelta::milliseconds(i64::deserialize(deserializer)?))
    }
}

pub mod timedelta_milliseconds_opt {
    use chrono::TimeDelta;
    use serde::{de, Deserialize, Deserializer, Serializer};

    pub fn serialize<S>(opt: &Option<TimeDelta>, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        match *opt {
            None => serializer.serialize_none(),
            Some(dur) => serializer.serialize_some(&dur.num_milliseconds()),
        }
    }

    pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<TimeDelta>, D::Error>
    where
        D: Deserializer<'de>,
    {
        deserializer.deserialize_option(OptionMillisecondTimeDeltaVisitor)
    }

    struct OptionMillisecondTimeDeltaVisitor;

    impl<'de> de::Visitor<'de> for OptionMillisecondTimeDeltaVisitor {
        type Value = Option<TimeDelta>;

        fn expecting(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result {
            formatter.write_str("Some(milliseconds) or None")
        }

        fn visit_none<E>(self) -> Result<Self::Value, E>
        where
            E: de::Error,
        {
            Ok(None)
        }

        fn visit_some<D>(self, d: D) -> Result<Self::Value, D::Error>
        where
            D: de::Deserializer<'de>,
        {
            let millis = i64::deserialize(d)?;
            Ok(Some(TimeDelta::milliseconds(millis)))
        }

        fn visit_unit<E>(self) -> Result<Self::Value, E>
        where
            E: de::Error,
        {
            Ok(None)
        }
    }
}

maybe somehere in https://docs.rs/chrono/latest/src/chrono/lib.rs.html#612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests