chrono
chrono copied to clipboard
Add support for parsing JSON timestamps from strings.
This change adds support for parsing string as (numeric) timestamps.
I had a look at the tests but was not sure where I should put the test for this feature, as the current logic to run tests is calling datetime::test_decodable_json which does not seem to be exercising the visitor path? Let me know where I should put them and I'll add the tests.
I think it's probably worthwhile having a few tests to verify this works. They could just be placed at the bottom of the serde.rs file along with the other tests there.
@rayhaanj would you have time to add a few tests?
I'll try to help get this PR ready.
Added some very basic doctests. Good this was requested in previous reviews, as it didn't work yet :smile:.
We now need to use deserialize_any instead of deserialize_i64.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 91.85%. Comparing base (
0cfc405) to head (7f60d2c).
Additional details and impacted files
@@ Coverage Diff @@
## main #926 +/- ##
==========================================
+ Coverage 91.80% 91.85% +0.04%
==========================================
Files 37 37
Lines 18148 18181 +33
==========================================
+ Hits 16661 16700 +39
+ Misses 1487 1481 -6
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hmm, desertialize_any may not be the best thing to reach for:
Require the
Deserializerto figure out how to drive the visitor based on what data type is in the input.When implementing
Deserialize, you should avoid relying onDeserializer::deserialize_anyunless you need to be told by theDeserializerwhat type is in the input. Know that relying onDeserializer::deserialize_anymeans your data type will be able to deserialize from self-describing formats only, ruling out Postcard and many others.
Yeah, I don't think we can/should restrict to self-describing formats only.
I've opened a question in the serde repo just in case. I think this is not going to work out, but no need to hurry closing this PR yet :smile:.
Closing as this approach seems like it is not going to work.