Skip to content
This repository has been archived by the owner on Sep 24, 2022. It is now read-only.

Add time/chrono to access date/time values #159

Closed
killercup opened this issue Apr 5, 2017 · 10 comments
Closed

Add time/chrono to access date/time values #159

killercup opened this issue Apr 5, 2017 · 10 comments

Comments

@killercup
Copy link

killercup commented Apr 5, 2017

Currently, date/time values are only represented by internal structures that only support conversion to/from strings. Adding the ability to convert these types to structures from the time and/or chrono crate would enable people to deal wth these types much more nicely.

cc Rustaceans/rust-cologne#29

@killercup
Copy link
Author

killercup commented Apr 5, 2017

Some advice from @dtolnay:

[20:40:29]  <killercup>	dtolnay: just so i (a serde noob) understand correctly, what would i need to do to parse some toml to a `struct Foo { date: chrono::NaiveDate }` (via `toml::Datetime`)?
[20:43:18]  <dtolnay>	looks like the deserialize impl for chrono::NaiveDate expects the input to contain a string https://github.com/chronotope/chrono/blob/v0.3.0/src/naive/date.rs#L1594-L1600
[20:43:38]  <dtolnay>	so if your toml data contains a date represented as a string, it will work just like that
[20:44:23]  <dtolnay>	if the toml data contains a date in toml's datetime representation, it would need to be `struct Foo { #[serde(deserialize_with = "from_toml_date") date: chrono::NaiveDate }`
[20:45:02]  <dtolnay>	where from_toml_date is a function that looks like `toml::Datetime::deserialize(deserializer).map(map it to chrono NaiveDate somehow)`
[20:48:26]  <dtolnay>	oh a better way to make it work would be to add a deserialize_str function to this impl https://github.com/alexcrichton/toml-rs/blob/0.3.2/src/de.rs#L135
[20:48:58]  <dtolnay>	and in the deserialize_str function, add logic to check whether the toml input data contains a datetime, and if so, return it as a string instead of the usual way it treats datetimes
[20:49:23]  <dtolnay>	then `struct Foo { date: chrono::NaiveDate }` would work
[20:49:51]  <dtolnay>	the toml input would contain a toml datetime, the NaiveDate deserialize impl would try to deserialize it as a string, and the toml Deserializer would lie to it and pretend like the input contains a string

@quadrupleslap
Copy link

quadrupleslap commented Nov 14, 2017

is this what needs to be done?

  1. Follow the deserialize_str hint in de.rs like @dtolnay said.
  2. Add conversion functions between Datetime and the various chrono types (DateTime<Utc>, DateTime<Local>, Date<Local> and NaiveTime.)
  3. Somehow serialize the chrono types to normal dates in the serializer.

Parts 1 and 2 sound easy, but to the serializer, aren't the chrono types indistinguishable from strings, making part 3 impossible? Also, now that I think about it, wouldn't the sensible option for deserialize_any on a Datetime be to make it a string, not the hidden struct thing?

Edit: The more I look at chrono's serde integration, the more it seems like it's doing the wrong thing by looking exactly like a string.

@alexcrichton
Copy link
Collaborator

@quadrupleslap oh I don't think we'll want a toml date/time to be directly deserializable to a chrono date/time, but rather what I think we'll want to do is to provide baked-in conversion methods for the Datetime type. That is, we'd have an optional dependency on chrono for example which when enabled would add accessors that return the appropriate chrono types given a Datetime object (and similarly constructors of Datetime from chrono types)

@quadrupleslap
Copy link

Oh, okay, but that means auto-derived Deserialize won't work for structs with chrono::DateTime inside them, which is a bit of a shame.

@quadrupleslap
Copy link

quadrupleslap commented Mar 6, 2018

@alexcrichton I need this again, so I'm trying to implement it. Should I just add chrono_time(&self) -> Option<chrono::NaiveTime>, chrono_time(&self) -> Option<chrono::NaiveDate> and chrono_tz(&self) -> Option<chrono::FixedOffset>? I'm asking because that doesn't sound very fun to use.

@alexcrichton
Copy link
Collaborator

@quadrupleslap ok awesome! I'm not too familiar with chrono so I'm fine w/ whatever works for chrono!

@quadrupleslap
Copy link

quadrupleslap commented Feb 14, 2019

This is a massive breaking change, but the Datetime type should probably be split up into DateTime, LocalDateTime, LocalTime and LocalDate. If you don't, the array can be heterogeneous, because it could include a LocalDate and a LocalTime.

(Also, it's strange how I keep needing this exactly once every year.)

@samuelcolvin
Copy link

In the absence of full chrono integration, would it be possible to expose currently private members of the datetime struct?

Currently in rtoml (python wrapper for this library) I'm having to use .to_string() when some pretty complex regexes to convert to a datetime object.

@alexcrichton
Copy link
Collaborator

Seems reasonable to me to add!

@epage
Copy link
Member

epage commented Sep 23, 2022

Maintenance of this crate has moved to the https://github.com/toml-rs/toml repo. As a heads up, we plan to move toml to be on top of toml_edit, see toml-rs/toml#340.

Closing this out. If this is still a problem, feel free to recreate this issue in the new repo.

@epage epage closed this as completed Sep 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants