From 2efc46a43ded5536bb57debebd37ab67d1b3eb58 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Mon, 21 Apr 2025 05:13:22 +0200 Subject: [PATCH 1/2] Refactor ConflictingField error to contain the previously loaded field --- components/datetime/src/pattern/mod.rs | 9 ++- .../datetime/src/scaffold/names_storage.rs | 69 ++++++++++++++++--- ffi/capi/src/errors.rs | 6 +- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/components/datetime/src/pattern/mod.rs b/components/datetime/src/pattern/mod.rs index b6d7e5d3ca4..1157cc41ff8 100644 --- a/components/datetime/src/pattern/mod.rs +++ b/components/datetime/src/pattern/mod.rs @@ -73,8 +73,13 @@ pub enum PatternLoadError { /// Fields conflict if they require the same type of data, for example the /// `EEE` and `EEEE` fields (short vs long weekday) conflict, or the `M` /// and `L` (format vs standalone month) conflict. - #[displaydoc("A field {0:?} conflicts with a previous field.")] - ConflictingField(ErrorField), + #[displaydoc("A field {field:?} conflicts with a previously loaded field {previous_field:?}.")] + ConflictingField { + /// The field that was not able to be loaded. + field: ErrorField, + /// The field that prevented the new field from being loaded. + previous_field: ErrorField, + }, /// The field symbol is not supported in that length. /// /// Some fields, such as `O` are not defined for all lengths (e.g. `OO`). diff --git a/components/datetime/src/scaffold/names_storage.rs b/components/datetime/src/scaffold/names_storage.rs index 1f4253860c0..ef945bf1ede 100644 --- a/components/datetime/src/scaffold/names_storage.rs +++ b/components/datetime/src/scaffold/names_storage.rs @@ -43,6 +43,45 @@ pub trait DateTimeNamesMarker: UnstableSealed { type MetazoneLookup: NamesContainer; } +/// A trait for `Variables` that can be converted to [`ErrorField`] +pub trait MaybeAsErrorField: UnstableSealed { + fn maybe_as_error_field(&self) -> Option; +} + +impl MaybeAsErrorField for () { + fn maybe_as_error_field(&self) -> Option { + None + } +} + +impl UnstableSealed for YearNameLength {} +impl MaybeAsErrorField for YearNameLength { + fn maybe_as_error_field(&self) -> Option { + Some(self.to_approximate_error_field()) + } +} + +impl UnstableSealed for MonthNameLength {} +impl MaybeAsErrorField for MonthNameLength { + fn maybe_as_error_field(&self) -> Option { + Some(self.to_approximate_error_field()) + } +} + +impl UnstableSealed for WeekdayNameLength {} +impl MaybeAsErrorField for WeekdayNameLength { + fn maybe_as_error_field(&self) -> Option { + Some(self.to_approximate_error_field()) + } +} + +impl UnstableSealed for DayPeriodNameLength {} +impl MaybeAsErrorField for DayPeriodNameLength { + fn maybe_as_error_field(&self) -> Option { + Some(self.to_approximate_error_field()) + } +} + /// Trait that associates a container for a payload parameterized by the given variables. /// ///
@@ -69,7 +108,7 @@ macro_rules! impl_holder_trait { impl UnstableSealed for $marker {} impl NamesContainer<$marker, Variables> for $marker where - Variables: PartialEq + Copy + fmt::Debug, + Variables: PartialEq + Copy + MaybeAsErrorField + fmt::Debug, { type Container = DataPayloadWithVariables<$marker, Variables>; } @@ -97,10 +136,10 @@ impl_holder_trait!(tz::MzPeriodV1); #[derive(Debug, displaydoc::Display)] #[non_exhaustive] pub enum MaybePayloadError { - /// TODO + /// The container's field set doesn't support the field FormatterTooSpecific, - /// TODO - ConflictingField, + /// The field is already loaded with a different length + ConflictingField(ErrorField), } impl core::error::Error for MaybePayloadError {} @@ -109,7 +148,10 @@ impl MaybePayloadError { pub(crate) fn into_load_error(self, error_field: ErrorField) -> PatternLoadError { match self { Self::FormatterTooSpecific => PatternLoadError::FormatterTooSpecific(error_field), - Self::ConflictingField => PatternLoadError::ConflictingField(error_field), + Self::ConflictingField(loaded_field) => PatternLoadError::ConflictingField { + field: error_field, + previous_field: loaded_field, + }, } } } @@ -200,7 +242,7 @@ where impl MaybePayload for DataPayloadWithVariables where - Variables: PartialEq + Copy, + Variables: PartialEq + Copy + MaybeAsErrorField, { #[inline] fn new_empty() -> Self { @@ -224,8 +266,19 @@ where // TODO(#6063): probably not correct return Ok(Ok(Default::default())); } - OptionalNames::SingleLength { .. } => { - return Err(MaybePayloadError::ConflictingField); + OptionalNames::SingleLength { variables, .. } => { + let loaded_field = match variables.maybe_as_error_field() { + Some(x) => x, + None => { + debug_assert!(false, "all non-unit variables implement this trait"); + use crate::provider::fields::*; + ErrorField(Field { + symbol: FieldSymbol::Era, + length: FieldLength::Six, + }) + } + }; + return Err(MaybePayloadError::ConflictingField(loaded_field)); } OptionalNames::None => (), }; diff --git a/ffi/capi/src/errors.rs b/ffi/capi/src/errors.rs index e2d05ba1a33..2ec60dd4e66 100644 --- a/ffi/capi/src/errors.rs +++ b/ffi/capi/src/errors.rs @@ -204,7 +204,7 @@ impl From for DateTimeFormatterLoadErr fn from(e: icu_datetime::DateTimeFormatterLoadError) -> Self { match e { icu_datetime::DateTimeFormatterLoadError::Names( - icu_datetime::pattern::PatternLoadError::ConflictingField(_), + icu_datetime::pattern::PatternLoadError::ConflictingField { .. }, ) => Self::ConflictingField, icu_datetime::DateTimeFormatterLoadError::Names( icu_datetime::pattern::PatternLoadError::UnsupportedLength(_), @@ -246,7 +246,9 @@ impl From for DateTimeFormatterLoadError { impl From for ffi::DateTimeFormatterLoadError { fn from(value: icu_datetime::pattern::PatternLoadError) -> Self { match value { - icu_datetime::pattern::PatternLoadError::ConflictingField(_) => Self::ConflictingField, + icu_datetime::pattern::PatternLoadError::ConflictingField { .. } => { + Self::ConflictingField + } icu_datetime::pattern::PatternLoadError::UnsupportedLength(_) => { Self::UnsupportedLength } From 3477bdd5a9f9162ed578fe8521737f035f839fa1 Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Mon, 21 Apr 2025 08:56:17 +0200 Subject: [PATCH 2/2] Docs tests --- components/datetime/src/pattern/names.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/components/datetime/src/pattern/names.rs b/components/datetime/src/pattern/names.rs index 5d71b0a3c3a..42ec3622791 100644 --- a/components/datetime/src/pattern/names.rs +++ b/components/datetime/src/pattern/names.rs @@ -1247,7 +1247,7 @@ impl FixedCalendarDateTimeNames FixedCalendarDateTimeNames FixedCalendarDateTimeNames { /// // But loading a new length fails: /// assert!(matches!( /// names.include_day_period_names(DayPeriodNameLength::Abbreviated), - /// Err(PatternLoadError::ConflictingField(_)) + /// Err(PatternLoadError::ConflictingField { .. }) /// )); /// ``` #[cfg(feature = "compiled_data")] @@ -1445,11 +1445,11 @@ impl FixedCalendarDateTimeNames { /// // But loading a new symbol or length fails: /// assert!(matches!( /// names.include_weekday_names(WeekdayNameLength::StandaloneWide), - /// Err(PatternLoadError::ConflictingField(_)) + /// Err(PatternLoadError::ConflictingField { .. }) /// )); /// assert!(matches!( /// names.include_weekday_names(WeekdayNameLength::Abbreviated), - /// Err(PatternLoadError::ConflictingField(_)) + /// Err(PatternLoadError::ConflictingField { .. }) /// )); /// ``` #[cfg(feature = "compiled_data")]