-
-
Notifications
You must be signed in to change notification settings - Fork 19
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 TimestampInterpretation for flexible timestamp parsing #66
base: main
Are you sure you want to change the base?
Conversation
…` variants. - Updated `TimeConfig` to include a `timestamp_interpretation` field. - Modified `TimeConfigBuilder` to allow setting the `timestamp_interpretation`.
…terpretation modes.
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
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 very much for working on this, overall looks great!
I have a few thoughts around the position of the config and some changes we can probably skip...
TimestampInterpretation::Auto => { | ||
if timestamp.abs() > 20_000_000_000 { | ||
(timestamp / 1000, ((timestamp % 1000) * 1000) as u32) | ||
} else { | ||
(timestamp, 0) | ||
} | ||
} | ||
TimestampInterpretation::AlwaysSeconds => (timestamp, 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.
I think here we don't need to handle this case, as the limit of valid input is 86,400 seconds, i.e 86,400,000 milliseconds which is less than 20,000,000,000.
So my reading is that valid input here will always be treated as seconds anyway.
(A comment saying why we don't use timestamp_interpretation
here might be useful.)
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.
You are right @davidhewitt, I will fix that
@@ -273,26 +269,40 @@ impl Time { | |||
/// assert_eq!(d.to_string(), "01:02:20.000123"); | |||
/// ``` | |||
pub fn from_timestamp_with_config( | |||
timestamp_second: u32, | |||
timestamp: i64, |
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.
Because of the limit of valid input I think the original u32
input made more sense.
timestamp: i64, | |
timestamp: u32, |
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.
working on it
/// Defines how timestamps should be interpreted | ||
pub timestamp_interpretation: TimestampInterpretation, |
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.
It feels to me like this is actually config for DateTime
, not Time
.
So I suggest we make a DatetimeConfig { time_config: TimeConfig, timestamp_interpretation: TimestampInterpretation }
in datetime.rs
It might also be worth trying out this branch against pydantic-core
to see how painful it is to adjust to needing a different DatetimeConfig
type.
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.
Thank you for the suggestion, @davidhewitt. I'll implement the DatetimeConfig
in datetime.rs
and aim to ensure backward compatibility. I'll test this branch against pydantic-core
and update you on the results.
Description
I added a new
TimestampInterpretation
enum and updates theTimeConfig
to allow us specify how timestamps should be interpreted. This change provides more flexibility in parsing timestamps, especially for systems that may receive timestamps in different formats.Changes
TimestampInterpretation
enum withAuto
andAlwaysSeconds
variants.TimeConfig
to include atimestamp_interpretation
field.TimeConfigBuilder
to allow setting thetimestamp_interpretation
.DateTime::from_timestamp_with_config
to handle different interpretation modes.How to Use
Setting up TimeConfig with TimestampInterpretation
Anyone can now specify how they want timestamps to be interpreted when creating a
TimeConfig
:Notes
Auto
mode behaves like the previous implementation, automatically detecting if a timestamp is in seconds or milliseconds based on its magnitude.AlwaysSeconds
mode always interprets the timestamp as seconds, which may result inDateTooLarge
errors for timestamps that would previously have been interpreted as milliseconds.