-
Notifications
You must be signed in to change notification settings - Fork 527
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
Deserializing using serde deserialize does not work for String timestamp #196
Comments
The good news: you are correct about the The tl;dr: I recommend that you copy/paste the I think that string timestamps are far enough away from a common occurrence that I'm hesitant to add code to support them to If you copy/paste the // untested!
fn visit_str<E>(self, value: &str) -> Result<DateTime<FixedOffset>, E>
where E: de::Error
{
let ts: i64 = value.parse()
.map_err(|e| E::custom(format!("Unable to parse timestamp as int: {}", e)))?;
self.visit_i64(ts)
} And then call that from your copied in deserialize function, you should be golden. Or at least a lighter shade than "burnt". |
Thanks for quick response @quodlibetor ! Awesome, that sounds great. Yeah I have no strong sense that sending timestamps as strings is something common, at very least I've stumbled upon them for the first time. But I imagine you can best decide whether it's useful to support them long term based on whether there are explicit asks for it. Copying important bits and adding |
Let me know how it goes, and if anything obvious comes up that it seems like chrono should make easier for this use case. |
Sounds great, thank you! Just tested it, works like a charm. For future reference and if anyone needs this, the gist is very simple: It just copies ts_seconds excluding some serialization bits and adds |
Thanks! |
Hello. Only in my case there were not seconds but milliseconds ("ts_milliseconds"). I don't know Rust that well, but maybe there is a way to cascade serde handlers? Something like String -> @hanlder-> Integer -> @ts_milliseconds-> DateTime. Something like ??? |
Yeah. String timestamps aren't that uncommon. Even Google uses them, e.g. in RTDN. |
I'm open to supporting this, please submit a PR. |
On it. |
Another solution we could consider is scrapping the serde timestamp code in this crate and instead telling people to use serde_with. They have an elegant solution using their #[serde_as]
#[derive(Deserialize, Serialize)]
struct Timestamps {
#[serde_as(as = "TimestampSeconds<i64>")]
dt_i64: DateTime<Utc>,
#[serde_as(as = "TimestampSeconds<f64>")]
dt_f64: DateTime<Local>,
#[serde_as(as = "TimestampSeconds<String>")]
dt_string: DateTime<Utc>,
}; Would that crate be suitable for all current applications of timestampt serialisation? In terms of:
|
I propose we close my current PR and instead:
Any objections? CC @pitdicker @djc |
I don't think there is a need to deprecate the existing serde modules. I see There are some advantages of the |
Hi @jonasbb , thanks for joining the discussion and for your work on the serde_with crate. The issue is that the way timestamp ser/de is done is very repetitive and verbose, and neither modular nor extensible. Each of the In my view, one of the following must be true:
If I detach myself from the pain of my work going to waste, I prefer option 3. The problem is already solved very satisfactorily in serde_with and it seems wasteful to reimplement it in chrono. Option 1 is also acceptable to me, though more work to maintain in the long run. |
None of these must be true because they're extreme positions to take. I find the repetitive implementations unattractive but not unacceptable -- clearly having less repetition is better but some repetition (in code that doesn't need many changes over time) isn't a big problem in my mind. On the need for supporting different kinds of timestamp, I feel that providing these in this crate would make life easier for downstream users, but it's clearly not a must-have since serde has enough affordances that custom implementations and implementations from third-party crates are an option. As such, if you're satisfied with using serde-with, just do that. If someone wants to come along and spend the time to get it right within this crate, that would be nice. |
Hello!
I was curious whether it's a bug, a design choice, or my incorrect usage of the framework and would definitely appreciate any help and guidance on this one!
I'm using chrono to support deserialization of time formats in my parsing layer. One of the APIs I am currently using sends me timestamps as string. Example
I'd like to represent said JSON in my struct as:
I noticed that there is an attempt to support such situation by using the following function:
https://github.com/chronotope/chrono/blob/master/src/datetime.rs#L750
Unfortunately I was not able to get that working with strings. The following implementation of struct does not end up parsing the JSON properly:
I was curious whether it's a bug or maybe my setup is plain wrong?
The one thing that I've noticed is that this Deserialize function directly uses
SecondsTimestampVisitor
to parse out the timestamps. That Visitor though, if I'm reading the code correctly, does not implement eithervisit_str
orvisit_string
. I'm not sure if that's intended.Sorry if it turns out to be a silly question, I'm still quite new to Rust! :)
The text was updated successfully, but these errors were encountered: