Extra serde modules #258
Extra serde modules #258
Conversation
src/datetime.rs
Outdated
| where E: de::Error | ||
| { | ||
| from(FixedOffset::east(0).timestamp_opt(value, 0), &value) | ||
| serde_from(Utc.timestamp_opt(value / 1_000_000_000, | ||
| (value % 1_000_000_000) as u32), |
There was a problem hiding this comment.
This fails if value is negative, because the modulus will return a negative number and casting it to u32 will make it exceed the maximal value of 999_999_999.
The same applies for the other deserializations of i64.
There was a problem hiding this comment.
I mean, that's how it was before. I didn't actually change that code, Git just says I did.
Would it be better to call abs on value before the modulus?
There was a problem hiding this comment.
I know that it is a previous issue. I just though I mention it, if the whole serialization part is changed here anyways.
I do not think abs alone works right. Suppose value is -1, then you want the two values passed to timestamp_opt to be -1 and 999_999_999, but just abs would give you 0 and 1.
There was a problem hiding this comment.
As the nanosecond argument of timestamp_opt is unsigned, there isn’t a way to directly pass a negative timestamp correctly anyway.
What I could do is check for a negative timestamp and offset the seconds by negative one second, then pass one second subtract the positive nanoseconds part for the nanoseconds argument? That would give the desired results.
There was a problem hiding this comment.
Same with the other signed timestamps.
There was a problem hiding this comment.
Sorry about the delay reviewing this!
That does seem reasonable, not handling negative timestamps is a... sad mistake, that I think might be duplicated through more of the codebase. Since this isn't you're fault I'm happy to merge this and fix the negative timestamp issue in a separate PR. Let me know what you think.
There was a problem hiding this comment.
Relatedly, I started trying to deal with negative timestamps in #252 but haven't had the time to finish it.
There was a problem hiding this comment.
So if we just assume positive timestamps for now, everything looks good?
|
Thanks for the patience, I'm fine keeping the existing bug and merging this. |
Sorry this took so long; it kind of got pushed to the side for a while.
This fleshes out the
serdemodules for seconds, milliseconds and nanoseconds for both UTCDataTimes andNaiveDateTimes.It also simplifies a few things that were shared across the modules.