Skip to content
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

Add support for parsing JSON timestamps from strings. #926

Closed
wants to merge 2 commits into from

Conversation

rayhaanj
Copy link

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.

@djc djc requested a review from esheppa January 9, 2023 13:52
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

@djc djc mentioned this pull request Feb 16, 2023
@esheppa
Copy link
Collaborator

esheppa commented Feb 16, 2023

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.

@djc
Copy link
Member

djc commented Mar 9, 2023

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

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs test cases.

@rayhaanj is it possible to add test cases for "before"? That is, test cases that exercise some serde code before your changes. Then another commit with your changes + new test cases (or test case changes).

@pitdicker pitdicker changed the base branch from 0.5.x to main April 6, 2024 17:44
This adds support for deserializing a timestamp from a String JSON type.

Such functionality is useful when interacting with JSON endpoints which
for whatever reason choose to represent a timestamp as a string.
@pitdicker
Copy link
Collaborator

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 😄.
We now need to use deserialize_any instead of deserialize_i64.

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pitdicker
Copy link
Collaborator

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.

@djc
Copy link
Member

djc commented Apr 6, 2024

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

@pitdicker
Copy link
Collaborator

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

@pitdicker
Copy link
Collaborator

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

@pitdicker pitdicker closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants