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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Remove VVV time zone format #5659

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66e885f
Remove VVV time zone format
sffc Oct 8, 2024
308cd45
rm ExemplarCityFormat
sffc Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions components/datetime/src/format/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,12 +739,6 @@ fn select_zone_units(time_zone: ResolvedNeoTimeZoneSkeleton) -> [Option<TimeZone
Some(TimeZoneFormatterUnit::GenericLocation),
Some(TimeZoneFormatterUnit::LocalizedOffsetLong),
],
// 'VVV'
ResolvedNeoTimeZoneSkeleton::City => [
Some(TimeZoneFormatterUnit::ExemplarCity),
Some(TimeZoneFormatterUnit::LocalizedOffsetLong),
None,
],
// 'VVVV'
ResolvedNeoTimeZoneSkeleton::Location => [
Some(TimeZoneFormatterUnit::GenericLocation),
Expand Down
16 changes: 3 additions & 13 deletions components/datetime/src/format/neo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,16 +1028,7 @@ impl<C: CldrCalendar, R: DateTimeNamesMarker> TypedDateTimeNames<C, R> {
/// .include_time_zone_exemplar_city_names()
/// .unwrap();
///
/// // Create a pattern with symbol `VVV`:
/// let pattern_str = "'Your time zone is:' VVV";
/// let pattern: DateTimePattern = pattern_str.parse().unwrap();
///
/// assert_try_writeable_eq!(
/// names.with_pattern(&pattern).format(&zone_london_winter),
/// "Your time zone is: London",
/// );
///
/// // Now try `VVVV`:
/// // Try `VVVV`:
/// let pattern_str = "'Your time zone is:' VVVV";
/// let pattern: DateTimePattern = pattern_str.parse().unwrap();
///
Expand Down Expand Up @@ -2068,9 +2059,8 @@ impl<R: DateTimeNamesMarker> RawDateTimeNames<R> {
locale,
)?;
}
// 'VVV..VVVV' (note: `V..VV` are for identifiers only)
ResolvedNeoTimeZoneSkeleton::City
| ResolvedNeoTimeZoneSkeleton::Location => {
// 'VVVV' (note: `V..VV` are for identifiers only)
ResolvedNeoTimeZoneSkeleton::Location => {
self.load_time_zone_essentials(zone_essentials_provider, locale)?;
self.load_fixed_decimal_formatter(
fixed_decimal_formatter_loader,
Expand Down
3 changes: 2 additions & 1 deletion components/datetime/src/time_zone.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use writeable::Writeable;
/// All time zone styles that this crate can format
#[derive(Debug, Copy, Clone)]
pub(crate) enum ResolvedNeoTimeZoneSkeleton {
City,
Location,
GenericShort,
GenericLong,
Expand All @@ -44,6 +43,7 @@ pub(crate) enum ResolvedNeoTimeZoneSkeleton {
IsoXXXXX,
// TODO:
// `VV` "America/Los_Angeles"
// `VVV` "Los Angeles"
// Generic Partial Location: "Pacific Time (Los Angeles)"
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

@sffc sffc Oct 8, 2024 •
edited
Loading

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?

Copy link
Member Author

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?

Fair point

Copy link
Member

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.

}

Expand Down Expand Up @@ -166,6 +166,7 @@ pub(super) enum TimeZoneFormatterUnit {
LocalizedOffsetLong,
LocalizedOffsetShort,
Iso8601(Iso8601Format),
#[allow(dead_code)]
ExemplarCity,
Copy link
Member

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

Bcp47Id,
}
Expand Down
1 change: 0 additions & 1 deletion components/datetime/src/tz_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ macro_rules! time_zone_style_registry {
(GenericShort, LowerV, One), // 'v'
(GenericLong, LowerV, Wide), // 'vvvv'
(Bcp47Id, UpperV, One), // 'V'
(City, UpperV, Abbreviated), // 'VVV'
(Location, UpperV, Wide), // 'VVVV'
(Isoxxxx, UpperZ, One), // 'Z'
(IsoXXXXX, UpperZ, Narrow), // 'ZZZZZ'
Expand Down
4 changes: 4 additions & 0 deletions components/datetime/tests/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,10 @@ fn test_time_zone_patterns() {
} in &test.expectations
{
for pattern_input in patterns {
if pattern_input == "VVV" {
// TODO(#5658): 'VVV' format not yet supported
continue;
}
let parsed_pattern = DateTimePattern::try_from_pattern_str(pattern_input).unwrap();
for expect in expected.iter() {
println!(".");
Expand Down
Loading