pydantic-core icon indicating copy to clipboard operation
pydantic-core copied to clipboard

Implement ser_json_temporal config option

Open ollz272 opened this issue 5 months ago • 10 comments
trafficstars

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

ollz272 avatar Jun 17 '25 09:06 ollz272

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

  1. Ignore ser_json_timedelta now
  2. if ser_json_timedelta is set, set ser_json_temporal to that value
  3. 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
  4. If both are set, always use ser_json_temporal

ollz272 avatar Jun 17 '25 09:06 ollz272

CodSpeed Performance Report

Merging #1743 will not alter performance

Comparing ollz272:ser_json_datetime (d852a1a) with main (d523cf5)

Summary

✅ 157 untouched benchmarks

codspeed-hq[bot] avatar Jun 17 '25 09:06 codspeed-hq[bot]

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.

davidhewitt avatar Jun 17 '25 11:06 davidhewitt

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?

ollz272 avatar Jun 17 '25 13:06 ollz272

please review

ollz272 avatar Jun 17 '25 13:06 ollz272

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 avatar Jun 19 '25 14:06 davidhewitt

~~@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!~~

ollz272 avatar Jun 21 '25 09:06 ollz272

@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

ollz272 avatar Jun 21 '25 10:06 ollz272

@davidhewitt was there anything else you wanted me to have a look at here?

ollz272 avatar Jun 26 '25 11:06 ollz272

@davidhewitt Sorry to ping - just wondering if you had some time to rereview this? Would love to get this in :-)

ollz272 avatar Jul 10 '25 09:07 ollz272

Sorry for dropping the ball here - will seek to complete review of both these PRs by EOD tomorrow.

davidhewitt avatar Jul 14 '25 15:07 davidhewitt

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

ollz272 avatar Jul 16 '25 10:07 ollz272