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 JsonSchemaAs impls for all Duration and Timestamp adaptors #685

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

swlynch99
Copy link
Contributor

@swlynch99 swlynch99 commented Jan 25, 2024

This PR picks up from where #683 left off.


Since there are quite a few different Duration and Timestamp adaptors most of this PR is just macro-driven copy paste. In earlier iterations it was much worse but with the help of an internal trait I was able to reduce it down to something manageable.

Details

This PR includes:

  • JsonSchemaAs impls for all DurationXXX and TimestampYYY adaptors for all formats and supported library types.
  • Tests for the above

Notes

  • Modelling Strict versions is easy. Their schema is just that of the underlying format.
  • Flexible adaptors required some judgement calls:
    • I have decided not to distinguish between integers and numbers - everything is typed as "instanceType": "number". I think this is more likely to be useful in practice than trying to make a distinction between "reads any number" and "emits only integers" - something that many consumers of the schema might not even consider a meaningful distinction.

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (f120ec3) 65.19% compared to head (cf29573) 65.29%.
Report is 5 commits behind head on master.

Files Patch % Lines
serde_with/src/schemars_0_8.rs 74.07% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   65.19%   65.29%   +0.10%     
==========================================
  Files          38       38              
  Lines        2287     2314      +27     
==========================================
+ Hits         1491     1511      +20     
- Misses        796      803       +7     

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

@jonasbb
Copy link
Owner

jonasbb commented Jan 28, 2024

Thank you. This looks good. To me the difference between number and integer does not seem that significant to me. Especially so, if this only causes an overgeneralization for the serializing (writing) case.

Yeah, the implementations for the Timestamp* and Duration* types is quite boilerplate heavy. The other implementations work very similar. There is a lot of macro generated code, that uses an internal representation and traits for converting.

One question about JSON schema: I saw that string types can have a pattern assigned for validating the content. For the types here that would be number like ([0-9]+) or decimal number ([0-9]+(\.[0-9]*)?). Is that worth adding? Otherwise I am happy to release the code as is.

The string representation of the date actually has only a restricted set
of values that can be used. By using the pattern property we can have
the json schema itself validate the string.

The regex used here for the pattern is a more lenient version of that
used to match numbers in JSON.
@swlynch99
Copy link
Contributor Author

The idea to use a pattern to validate the content of the string is a good one! I have added this. The actual regex is a bit more complicated because you can have an exponent in the number (e.g. 1e44 or 1.88e-3) but I reused the one from the json specification with some tweaks so it should work in pretty much all cases.

@jonasbb jonasbb merged commit dca18c4 into jonasbb:master Jan 30, 2024
24 checks passed
@swlynch99 swlynch99 deleted the schemars-duration branch January 31, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants