-
Notifications
You must be signed in to change notification settings - Fork 243
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
Adds a ser_json_datetime config option for configuring how datetimes are serialized #1465
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1465 +/- ##
==========================================
- Coverage 90.21% 89.26% -0.95%
==========================================
Files 106 112 +6
Lines 16339 17896 +1557
Branches 36 41 +5
==========================================
+ Hits 14740 15975 +1235
- Misses 1592 1901 +309
- Partials 7 20 +13
... and 46 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #1465 will not alter performanceComparing Summary
|
…ydantic-core into add-datetime-serializer
please review |
|
||
pub(crate) fn datetime_to_string(py_dt: &Bound<'_, PyDateTime>) -> PyResult<String> { | ||
pydatetime_as_datetime(py_dt).map(|dt| dt.to_string()) | ||
} | ||
|
||
pub(crate) fn datetime_to_seconds(py_dt: &Bound<'_, PyDateTime>) -> PyResult<i64> { | ||
pydatetime_as_datetime(py_dt).map(|dt| dt.timestamp()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, seems to be an issue here :(
Looks like dt.timestamp() here doesn't take into account the number of microseconds in the datetime, so anything under a second resolution will get ignored.
Looks like its a speed date thing:
https://github.com/pydantic/speedate/blob/d15ac62b7e98d6e4b8a769b6f5ffab2ad1db5ca5/src/datetime.rs#L580-L582
https://github.com/pydantic/speedate/blob/main/src/time.rs#L388-L393
So i guess a few options:
- We don't care, ignore (i dont think this is right)
- We care and can calculate the timestamp ourselves here
- We care and need to add something upstream to speeddate to allow for returning the timestamp as a f64 with microsecond precision
🤷🏻
tests/serializers/test_any.py
Outdated
'milliseconds_int', | ||
), | ||
( | ||
datetime(2024, 1, 1, 0, 0, 0, 100, ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this situation fails :(
timedelta_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_float'` or `'milliseconds_float'`. | ||
datetime_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_int'` or `'milliseconds_int'`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timedelta_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_float'` or `'milliseconds_float'`. | |
datetime_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_int'` or `'milliseconds_int'`. | |
timedelta_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_float'` or `'milliseconds_float'`. | |
datetime_mode: How to serialize `datetime` objects, either `'iso8601'`, `'seconds_int'` or `'milliseconds_int'`. |
@@ -454,6 +457,7 @@ def to_jsonable_python( | |||
exclude_none: Whether to exclude fields that have a value of `None`. | |||
round_trip: Whether to enable serialization and validation round-trip support. | |||
timedelta_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_float'`, or`'milliseconds_float'`. | |||
datetime_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_int'`, or`'milliseconds_int'`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datetime_mode: How to serialize `timedelta` objects, either `'iso8601'`, `'seconds_int'`, or`'milliseconds_int'`. | |
datetime_mode: How to serialize `datetime` objects, either `'iso8601'`, `'seconds_int'`, or`'milliseconds_int'`. |
Closing for now, but we're going to implement this in v2.11 :). |
Change Summary
Related to an issue i've opened in pydantic, adds a new config option to customise how pydantic serializes datetimes.
We now from this PR have 3 options:
iso8601
, which retains existing behaviourseconds_int
, which returns the timestamp of the datetime in secondsmilliseconds_int
, which returns the timestamp of the datetime in millisecondsThis would be a worthwhile feature for us to have, so hope this is okay to implement!
Related issue number
pydantic/pydantic#10454
Checklist
pydantic-core
(except for expected changes)Selected Reviewer: @sydney-runkle