Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

doziestar
Copy link

Description

I added a new TimestampInterpretation enum and updates the TimeConfig 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

  1. Added TimestampInterpretation enum with Auto and AlwaysSeconds variants.
  2. Updated TimeConfig to include a timestamp_interpretation field.
  3. Modified TimeConfigBuilder to allow setting the timestamp_interpretation.
  4. Updated 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:

use speedate::{TimeConfigBuilder, TimestampInterpretation};

// Auto mode (default behavior)
let auto_config = TimeConfigBuilder::new()
    .timestamp_interpretation(TimestampInterpretation::Auto)
    .build();

// Always interpret as seconds
let always_seconds_config = TimeConfigBuilder::new()
    .timestamp_interpretation(TimestampInterpretation::AlwaysSeconds)
    .build();

// This will be interpreted as milliseconds in `Auto` mode
let auto_dt = DateTime::from_timestamp_with_config(1654619320123, 0, &auto_config).unwrap();
assert_eq!(auto_dt.to_string(), "2022-06-07T16:28:40.123000");

// This will be interpreted as seconds in `AlwaysSeconds` mode
// Please Take Note: This might result in a `DateTooLarge` error for very large timestamps
let always_seconds_dt = DateTime::from_timestamp_with_config(1654619320, 0, &always_seconds_config).unwrap();
assert_eq!(always_seconds_dt.to_string(), "2022-06-07T16:28:40");
   

Notes

  • The Auto mode behaves like the previous implementation, automatically detecting if a timestamp is in seconds or milliseconds based on its magnitude.
  • The AlwaysSeconds mode always interprets the timestamp as seconds, which may result in DateTooLarge errors for timestamps that would previously have been interpreted as milliseconds.

…` variants.

- Updated `TimeConfig` to include a `timestamp_interpretation` field.
- Modified `TimeConfigBuilder` to allow setting the `timestamp_interpretation`.
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Files Patch % Lines
src/time.rs 97.14% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@davidhewitt davidhewitt left a 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...

Comment on lines +277 to +285
TimestampInterpretation::Auto => {
if timestamp.abs() > 20_000_000_000 {
(timestamp / 1000, ((timestamp % 1000) * 1000) as u32)
} else {
(timestamp, 0)
}
}
TimestampInterpretation::AlwaysSeconds => (timestamp, 0),
};
Copy link
Contributor

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.)

Copy link
Author

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,
Copy link
Contributor

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.

Suggested change
timestamp: i64,
timestamp: u32,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working on it

Comment on lines +629 to +630
/// Defines how timestamps should be interpreted
pub timestamp_interpretation: TimestampInterpretation,
Copy link
Contributor

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants