-
Notifications
You must be signed in to change notification settings - Fork 182
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 TimeZoneFallbackOverride date field length; use NeoTimeZoneSkeleton #5024
Conversation
I'm not completely sure which direction to take on this. The key issue here is that expressing configurations for time zone formatting is substantially thicker than for other fields. For example, if you need to format a month, you might first try to get the long symbol, then if that fails, short, and then finally numeric. However, for time zones, you might first try to format short generic based on BCP-47, then short generic based on Metazone, then long generic from BCP-47, then long generic from Metazone, then partial location format based on BCP-47, and then finally one of the handful of GMT offset fallback formats. Importantly, this set of fallback pathways can be configured by the user. There are two general ways to approach this:
This PR currently implements option 2ii. Prior to this PR, something closer to option 1 was implemented. I'm currently leaning toward sticking with option 1, in which case I'll update this PR to save some refactorings and revert others. Thoughts? Any other options I'm missing? @Manishearth @robertbastian @zbraniecki |
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'm not sure what preference I have here. The private use fields are a bit messy but not terrible. This also seems fine?
Interested in what Zibi thinks
components/datetime/src/neo_zone.rs
Outdated
|
||
/// "GMT-8" | ||
#[derive(Debug)] | ||
pub enum GmtShort {} |
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.
thought: not sure if empty enums are what we need for these markers but that's a separate discussion to be had later
I'm leaning toward private use field lengths (option 1) unless I hear otherwise. All the code is just so intertwined with PatternItems (for good reason), and there's not a good reason to break from that. |
674a1ea
to
5266c6c
Compare
5266c6c
to
bb378f1
Compare
Okay I've tried implementing option 1. Some more thoughts:
|
I don't have a strong opinion, I'm sorry. Can we do the Option 2 pre-processing at data build time? |
Ok this has been rewritten multiple times but I simplified the change to only use the private use field length and it's now ready for review. |
Backup of neo_zone.rs: // This file is part of ICU4X. For terms of use, please see the file
// called LICENSE at the top level of the ICU4X source tree
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).
//! Types for neo time zone formatting.
use crate::fields::{self, Field, FieldSymbol, FieldLength};
#[cfg(feature = "experimental")]
use crate::neo_marker::private;
use crate::time_zone::Iso8601Format;
/// Time zone offset formatting style.
#[derive(Debug, Copy, Clone)]
pub enum NeoOffsetStyle {
/// "GMT-8"
/// `O`
GmtShort,
/// "GMT-08:00"
/// `OOOO`, `ZZZZ`
GmtLong,
/// All `x` and `X` formats
Iso8601(Iso8601Format),
}
/// Time zone formatting style, not including offsets.
#[derive(Debug, Copy, Clone)]
pub enum NeoZoneStyle {
/// "PDT"
/// `z..zzz`
SpecificShort,
/// "Pacific Daylight Time"
/// `zzzz`
SpecificLong,
/// "PT"
/// `v`
GenericShort,
/// "Pacific Time"
/// `vvvv`
GenericLong,
/// "uslax"
/// `V`
IdentifierShort,
/// "America/Los_Angeles"
/// `VV`
IdentifierLong,
/// "Los Angeles"
/// `VVV`
City,
/// "Los Angeles Time"
/// `VVVV`
GenericLocation,
/// "Pacific Time (Los Angeles)"
/// not available via field?
GenericPartialLocation,
}
/// A specification for the desired display of a time zone.
///
/// Unlike date and time, we support only a single time zone component, but the
/// specific component can change.
///
/// Since the length of a time zone can vary independent of the date and time,
/// the time zone lengths are directly encoded into this enum.
#[derive(Debug, Copy, Clone)]
#[non_exhaustive]
pub struct NeoZoneConfig {
/// The primary time zone style.
pub first: Option<NeoZoneStyle>,
/// The fallback time zone style if the primary style is unavailable.
pub second: Option<NeoZoneStyle>,
/// The offset style if the time zone cannot be formatted.
pub offset: NeoOffsetStyle,
}
impl NeoZoneConfig {
pub(crate) fn from_field(
field: fields::TimeZone,
length: fields::FieldLength,
) -> Option<Self> {
match (field, length) {
// TODO: This should actually be GenericShortOrLocation according to spec
(fields::TimeZone::LowerV, fields::FieldLength::One) => Some(GenericShortOrGmt::ZONE_CONFIG),
_ => todo!()
}
}
pub(crate) fn default_for_fallback() -> Self {
Self {
first: None,
second: None,
offset: NeoOffsetStyle::Iso8601(Iso8601Format::default_for_fallback()),
}
}
}
/// A type that has a time zone configuration associated with it.
// TODO: Make this require sealed
pub trait HasZoneComponents {
/// The configuration for formatting this time zone.
const ZONE_CONFIG: NeoZoneConfig;
/// The corresponding field.
const ZONE_FIELD: Field;
}
/// "GMT-8"
#[derive(Debug)]
pub enum GmtShort {}
#[cfg(feature = "experimental")]
impl private::Sealed for GmtShort {}
impl HasZoneComponents for GmtShort {
const ZONE_CONFIG: NeoZoneConfig = NeoZoneConfig {
first: None,
second: None,
offset: NeoOffsetStyle::GmtShort,
};
const ZONE_FIELD: Field = Field {
symbol: FieldSymbol::TimeZone(fields::TimeZone::UpperO),
length: FieldLength::One,
};
}
/// "PT" or "GMT-8"
#[derive(Debug)]
pub enum GenericShortOrGmt {}
#[cfg(feature = "experimental")]
impl private::Sealed for GenericShortOrGmt {}
impl HasZoneComponents for GenericShortOrGmt {
const ZONE_CONFIG: NeoZoneConfig = NeoZoneConfig {
first: Some(NeoZoneStyle::GenericShort),
second: None,
offset: NeoOffsetStyle::GmtShort,
};
const ZONE_FIELD: Field = Field {
symbol: FieldSymbol::TimeZone(fields::TimeZone::LowerV),
length: FieldLength::TimeZoneFallbackOverride(fields::TimeZoneFallbackOverride::ShortOrGmt),
};
}
/// "Los Angeles Time" or "GMT-8"
#[derive(Debug)]
pub enum GenericLocationOrGmt {}
#[cfg(feature = "experimental")]
impl private::Sealed for GenericLocationOrGmt {}
impl HasZoneComponents for GenericLocationOrGmt {
const ZONE_CONFIG: NeoZoneConfig = NeoZoneConfig {
first: Some(NeoZoneStyle::GenericLocation),
second: None,
offset: NeoOffsetStyle::GmtShort,
};
const ZONE_FIELD: Field = Field {
symbol: FieldSymbol::TimeZone(fields::TimeZone::UpperV),
length: FieldLength::Wide,
};
}
/// "PT" or "Los Angeles Time" or "GMT-8"
#[derive(Debug)]
pub enum GenericShortOrLocation {}
#[cfg(feature = "experimental")]
impl private::Sealed for GenericShortOrLocation {}
impl HasZoneComponents for GenericShortOrLocation {
const ZONE_CONFIG: NeoZoneConfig = NeoZoneConfig {
first: Some(NeoZoneStyle::GenericShort),
second: Some(NeoZoneStyle::GenericLocation),
offset: NeoOffsetStyle::GmtShort,
};
const ZONE_FIELD: Field = Field {
symbol: FieldSymbol::TimeZone(fields::TimeZone::LowerV),
length: FieldLength::One,
};
} |
#1317