-
Notifications
You must be signed in to change notification settings - Fork 214
Add test illustrating mismatched time zone time and zoned datetime #6239
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
I don't think this is a bug, and I don't think absolute timestamps are the solution here, because they don't solve the problem of having two inputs that need to agree. We could change the I'm leaning more and more in the direction of having |
Discussed: OK to ignore this edge case in 2.0 |
I think it is still useful to land this PR. |
components/datetime/src/combo.rs
Outdated
#[cfg(test)] | ||
mod tests { | ||
use icu_calendar::Iso; | ||
use icu_locale_core::locale; | ||
use icu_time::{ | ||
zone::{IanaParser, UtcOffset, UtcOffsetCalculator}, | ||
DateTime, ZonedDateTime, | ||
}; | ||
use writeable::assert_writeable_eq; | ||
|
||
use crate::{ | ||
fieldsets::{zone::SpecificLong, YMDT}, | ||
DateTimeFormatter, | ||
}; | ||
|
||
#[test] | ||
fn please_dont_do_this() { |
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.
nit: useless test module
#[cfg(test)] | |
mod tests { | |
use icu_calendar::Iso; | |
use icu_locale_core::locale; | |
use icu_time::{ | |
zone::{IanaParser, UtcOffset, UtcOffsetCalculator}, | |
DateTime, ZonedDateTime, | |
}; | |
use writeable::assert_writeable_eq; | |
use crate::{ | |
fieldsets::{zone::SpecificLong, YMDT}, | |
DateTimeFormatter, | |
}; | |
#[test] | |
fn please_dont_do_this() { | |
#[test] | |
fn please_dont_do_this() { | |
use icu_calendar::Iso; | |
use icu_locale_core::locale; | |
use icu_time::{ | |
zone::{IanaParser, UtcOffset, UtcOffsetCalculator}, | |
DateTime, ZonedDateTime, | |
}; | |
use writeable::assert_writeable_eq; | |
use crate::{ | |
fieldsets::{zone::SpecificLong, YMDT}, | |
DateTimeFormatter, | |
}; |
Co-authored-by: Robert Bastian <[email protected]>
I realized that ZonedDateTime can contain two different datetimes: the "main" datetime and the reference datetime for the time zone.
It's fairly easy to get into this state. For example, someone might pick "Unix epoch" or "current datetime" to put into the fairly obscure
at_time
function when constructing their TimeZoneInfo, which is almost always wrong.This might be partially mitigated if we change the reference datetime to be an absolute time (#6238) so that there isn't a categorical mismatch, but it can still happen.
Maybe fine to ignore. If people use
infer_zone_offsets
, they won't get a specific non-location zone name that is wrong, only not applicable at the given time, which is confusing but not wrong. (However, if they pull the zone variant fromisdst
, maybe things could go wrong.)@robertbastian