-
Notifications
You must be signed in to change notification settings - Fork 293
Implement val_temporal_unit for deciding how datetimes and dates timestamps get validated. #1751
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #1751 will not alter performanceComparing Summary
|
…econds and infer to show behaviour
Note - added a test here as this is the pr I discovered the issue with fractional millisecond timestamps (pydantic/pydantic#12027) |
Please review |
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.
Thanks for working on this, and sorry for the slow review.
Overall looks great, with just a couple thoughts.
We decide that it should support time/duration, but its fine to get this part done first and add support for those later
This feels ok to me, let's just sense check:
- presumably for milliseconds
time
values these can never overlap with secondstime
values due to the limit of time being 24 hours. So this is safe to go in later as a new feature (would make values previously not accepted be valid). - for duration, the scale is presumably the same as datetime so we should consider if it makes sense to have inference there at all. IIRC the milliseconds scale kicks in for a timestamp which is on the order of hundreds of years? (we should sense check this). So it's unlikely but possible that this changes the meaning of valid values. In which case implementing this might be a separate issue we should consider for v3?
microseconds_overflow_behavior: MicrosecondsPrecisionOverflowBehavior, | ||
mode: TemporalUnitMode, |
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.
I wonder if it makes sense to just pass &DateTimeConfig
here to simplify things?
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.
I think i chose not to as i wasn't a fan of using something from the serialization module in the validation logic. Happy to be overridden though!
// Use the same watershed from speedate to determine if we treat the float as seconds or milliseconds. | ||
// TODO: should we expose this from speedate? | ||
if timestamp.abs() <= 20_000_000_000.0 { |
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.
Potentially worth adding a utility function in speedate DateTime::from_float_with_config
?
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.
Both make sense to me 👍🏻 |
Change Summary
Follow up to #1743 and a part of pydantic/pydantic#11504 implementation - allows the config switch val_temporal_unit to be used to determine how dates and datetimes timestamps are validated - either as seconds, milliseconds, or inferred.
NOTE - the original ticket makes mention of this also working for time and duration. As far as i can see, we do not currently infer the value of these, they're just assumed to be seconds. Theres no support for this in speedate anyway. So I've implemented what is currently possible. I see 3 possible outcomes from this:
Related issue number
Checklist
pydantic-core
(except for expected changes)Selected Reviewer: @davidhewitt