pydantic-core
pydantic-core copied to clipboard
Implement ser_json_temporal config option
Implements the first part of https://github.com/pydantic/pydantic/pull/11504, which is the ser_json_temporal config option.
We now have a ser_json_temporal part to the config, which accepts ['iso8601', 'seconds', 'milliseconds']. These have been implemented as designed in the original pydantic pull request ('iso8601' being obvious, seconds and milliseconds each returning the number of seconds and milliseconds in a float).
This has been done by implementing the serializers for each of the specified types (datetime, date, time, timedelta).
Special case for 'timedelta' where if the old config option is set, it will be used instead of ser_json_temporal.
Related issue number
https://github.com/pydantic/pydantic/pull/11504
Checklist
- [x ] Unit tests for the changes exist
- [x ] Documentation reflects the changes where applicable
- [ ] Pydantic tests pass with this
pydantic-core(except for expected changes) - [ ] My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers
Selected Reviewer: @sydney-runkle
Codecov Report
Attention: Patch coverage is 3.44828% with 252 lines in your changes missing coverage. Please review.
:loudspeaker: Thoughts on this report? Let us know!
@davidhewitt think i've got most of the way with the serialization part, just a question: https://github.com/pydantic/pydantic-core/pull/1743/files#diff-c465892d6081b2cc8bd4dbc610f0898a2591a8e53a7f3918dd13a54747f708fbR100-R105
https://github.com/pydantic/pydantic/pull/11504
The pydantic PR mentions the new config option overlaps with ser_json_timedelta, which will be deprecated, but doesn't (at least i can't see where) how we want to handle the case where both are set.
Do we want to do:
- Ignore ser_json_timedelta now
- if ser_json_timedelta is set, set ser_json_temporal to that value
- Have logic here where if ser_json_timedelta and ser_json_temporal is set, prefer using ser_json_timedelta over ser_json_temporal for timedeltas
- If both are set, always use ser_json_temporal
CodSpeed Performance Report
Merging #1743 will not alter performance
Comparing ollz272:ser_json_datetime (d852a1a) with main (d523cf5)
Summary
✅ 157 untouched benchmarks
Thanks!
I would make it such that if ser_json_temporal is set, use that value, otherwise use ser_json_timedelta value (where applicable).
That should make it non-breaking for existing users while adding the new preferred option.
Right @davidhewitt, think i've implemented everything now. Got tests that show everything is working. Not 100% sure on my implementation on deciding between timedelta_mode and temporal_mode, but that should be all the serialisation work done?
As a note the validation part of this seems a lot more complicated so may take a bit (lot) more time to implement.. hopefully supporting this first isn't too bad?
please review
As a note the validation part of this seems a lot more complicated so may take a bit (lot) more time to implement.. hopefully supporting this first isn't too bad?
I'm happy with starting here, the validation is a separate option, after all.
~~@davidhewitt I think i've addressed all your comments bar the temporal_mode bits on the timedelta, keep trying things but nothing seems right so i think im doing something wrong!~~
@davidhewitt actually, think i got the gist of it, didn't use the option<TimedeltaMode> in the end just create a method that turns the timedeltamode into a temporal mode for timedelta ser. Think this is everything resolved now
@davidhewitt was there anything else you wanted me to have a look at here?
@davidhewitt Sorry to ping - just wondering if you had some time to rereview this? Would love to get this in :-)
Sorry for dropping the ball here - will seek to complete review of both these PRs by EOD tomorrow.
Looks great; sorry again that this took some time to get around to.
Thanks @davidhewitt! once theres a release out for this i'll get support for this param into pydantic