chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Add support for parsing JSON timestamps from strings.

Open rayhaanj opened this issue 2 years ago • 2 comments
trafficstars

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.

rayhaanj avatar Dec 30 '22 01:12 rayhaanj

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.

esheppa avatar Feb 16 '23 12:02 esheppa

@rayhaanj would you have time to add a few tests?

djc avatar Mar 09 '23 09:03 djc

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.

pitdicker avatar Apr 06 '24 18:04 pitdicker

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.

codecov[bot] avatar Apr 06 '24 18:04 codecov[bot]

Hmm, desertialize_any may not be the best thing to reach for:

Require the Deserializer to figure out how to drive the visitor based on what data type is in the input.

When implementing Deserialize, you should avoid relying on Deserializer::deserialize_any unless you need to be told by the Deserializer what type is in the input. Know that relying on Deserializer::deserialize_any means your data type will be able to deserialize from self-describing formats only, ruling out Postcard and many others.

pitdicker avatar Apr 06 '24 18:04 pitdicker

Yeah, I don't think we can/should restrict to self-describing formats only.

djc avatar Apr 06 '24 19:04 djc

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:.

pitdicker avatar Apr 06 '24 20:04 pitdicker

Closing as this approach seems like it is not going to work.

pitdicker avatar Apr 10 '24 12:04 pitdicker