-
Notifications
You must be signed in to change notification settings - Fork 184
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
Remove VVV time zone format #5659
Conversation
components/datetime/src/time_zone.rs
Outdated
@@ -44,6 +43,7 @@ pub(crate) enum ResolvedNeoTimeZoneSkeleton { | |||
IsoXXXXX, | |||
// TODO: | |||
// `VV` "America/Los_Angeles" | |||
// `VVV` "Los Angeles" | |||
// Generic Partial Location: "Pacific Time (Los Angeles)" |
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: remove this, it's implemented
actually, do we need the full exemplar city names for it?
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.
hmm, that's annoying. Maybe not though because it seems Generic Partial Location is only used when you're not the primary zone?
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 it's fine, because #5657 uses the new logic for the bracketed part, i.e. it will say Central European Time (Germany)
and Pacific Time (Los Angeles)
.
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 would actually be nicer to say Pacific Time (United States)
, but I'd have to check if the metazone invariants allow that.
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.
Pacific Time (United States)
sounds extremely weird.
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.
Ah, Mountain Time (United States)
doesn't work.
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.
Pacific Time (United States) sounds extremely weird.
Weirder than Pacific Time (Los Angeles)
? It's the Pacific as used in the United States, following United States DST rules. They're not Los Angeles DST rules. Does Pacific Time (Mexico)
sound weird to you as well?
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.
Ah,
Mountain Time (United States)
doesn't work.
I think we're safe there because we only do the region override if the time zone is the only one in the country, so the city and region are interchangeable. Right?
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.
Pacific Time (United States) sounds extremely weird.
Weirder than
Pacific Time (Los Angeles)
? It's the Pacific as used in the United States, following United States DST rules. They're not Los Angeles DST rules. DoesPacific Time (Mexico)
sound weird to you as well?
Fair point
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 we're safe there because we only do the region override if the time zone is the only one in the country, so the city and region are interchangeable. Right?
Yes, current behaviour is safe. The proposed behaviour of Pacific Time (Canada)
needs more work, because we need to know whether a metazone+country combination is unique. ampa
+US
is, ammo
+US
isn't.
components/datetime/src/time_zone.rs
Outdated
@@ -166,6 +166,7 @@ pub(super) enum TimeZoneFormatterUnit { | |||
LocalizedOffsetLong, | |||
LocalizedOffsetShort, | |||
Iso8601(Iso8601Format), | |||
#[allow(dead_code)] | |||
ExemplarCity, |
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.
issue: remove this and ExemplarCityFormat
, because that is what's actually blocking
I'll let you merge this @robertbastian if you conclude that it is what we need for #5657 |
Unblocks #987
See #5658