Implement timestamp unit infer for other types#94
Conversation
| time_config: self.time_config.unwrap_or_default(), | ||
| time_config: self | ||
| .time_config | ||
| .unwrap_or_else(|| TimeConfigBuilder::new().timestamp_unit(TimestampUnit::Second).build()), |
There was a problem hiding this comment.
Tests pass also if I use the default (which will use TimestampUnit::Infer), but I'm trying to be extra safe here and make the config match the existing default behavior (see below).
There was a problem hiding this comment.
Ideally, the time config attribute of the datetime config should only allow TimestampUnit::second..
There was a problem hiding this comment.
We could also just remove timestamp_unit from DateTimeConfig so that it's not possible for there to be inconsistency? Would be breaking, but I think not too bad for users to adapt to.
Infer and Second are presumably equivalent for Time parsing, because the watershed is far out of range of a valid 24 hours.
| ) -> Result<Self, ParseError> { | ||
| let (mut second, extra_microsecond) = timestamp_to_seconds_micros(timestamp, config.timestamp_unit)?; | ||
| let (mut second, extra_microsecond) = | ||
| timestamp_to_seconds_micros(timestamp, config.timestamp_unit, MS_WATERSHED)?; |
There was a problem hiding this comment.
..If you expand below, you'll see that we call Time::from_timestamp_with_config() for the time component (which before this PR expected seconds as input). That's why as per the previous comment we need to make sure TimestampUnit::Second is used.
There was a problem hiding this comment.
Maybe we could have Time::from_seconds_timestamp_with_config as a private API to avoid that possible wrinkle (which would basically be the implementation before this PR, and from_timestamp_with_config is a public wrapper which processes the value and calls from_seconds_timestamp_with_config).
2 participants
No description provided.