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

Only load symbols data if pattern requires them. #791

Merged
merged 1 commit into from
Jun 18, 2021
Merged

Only load symbols data if pattern requires them. #791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 30 additions & 29 deletions components/datetime/src/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

use crate::{
fields::FieldSymbol,
format::datetime,
options::DateTimeFormatOptions,
pattern::PatternItem,
provider::{
gregory::{DatePatternsV1Marker, DateSymbolsV1Marker},
helpers::DateTimePatterns,
Expand Down Expand Up @@ -62,7 +60,7 @@ use crate::{
pub struct DateTimeFormat<'d> {
pub(super) locale: Locale,
pub(super) pattern: Pattern,
pub(super) symbols: DataPayload<'d, 'd, DateSymbolsV1Marker>,
pub(super) symbols: Option<DataPayload<'d, 'd, DateSymbolsV1Marker>>,
}

impl<'d> DateTimeFormat<'d> {
Expand Down Expand Up @@ -99,17 +97,6 @@ impl<'d> DateTimeFormat<'d> {
options: &DateTimeFormatOptions,
) -> Result<Self, DateTimeFormatError> {
let locale = locale.into();
let symbols_data = data_provider
.load_payload(&DataRequest {
resource_path: ResourcePath {
key: provider::key::GREGORY_DATE_SYMBOLS_V1,
options: ResourceOptions {
variant: None,
langid: Some(locale.clone().into()),
},
},
})?
.take_payload()?;

let patterns_data: icu_provider::DataPayload<
'_,
Expand All @@ -132,18 +119,26 @@ impl<'d> DateTimeFormat<'d> {
.get_pattern_for_options(options)?
.unwrap_or_default();

let time_zone_field = pattern
.items()
.iter()
.filter_map(|p| match p {
PatternItem::Field(field) => Some(field),
_ => None,
})
.find(|field| matches!(field.symbol, FieldSymbol::TimeZone(_)));
let requires_data = datetime::analyze_pattern(&pattern, false)
.map_err(|field| DateTimeFormatError::UnsupportedField(field.symbol))?;

if let Some(field) = time_zone_field {
return Err(DateTimeFormatError::UnsupportedField(field.symbol));
}
let symbols_data = if requires_data {
Some(
data_provider
.load_payload(&DataRequest {
resource_path: ResourcePath {
key: provider::key::GREGORY_DATE_SYMBOLS_V1,
options: ResourceOptions {
variant: None,
langid: Some(locale.clone().into()),
},
},
})?
.take_payload()?,
)
} else {
None
};

Ok(Self::new(locale, pattern, symbols_data))
}
Expand All @@ -164,7 +159,7 @@ impl<'d> DateTimeFormat<'d> {
pub(super) fn new<T: Into<Locale>>(
locale: T,
pattern: Pattern,
symbols: DataPayload<'d, 'd, DateSymbolsV1Marker>,
symbols: Option<DataPayload<'d, 'd, DateSymbolsV1Marker>>,
) -> Self {
let locale = locale.into();

Expand Down Expand Up @@ -209,7 +204,7 @@ impl<'d> DateTimeFormat<'d> {
{
FormattedDateTime {
pattern: &self.pattern,
symbols: self.symbols.get(),
symbols: self.symbols.as_ref().map(|s| s.get()),
datetime: value,
locale: &self.locale,
}
Expand Down Expand Up @@ -246,8 +241,14 @@ impl<'d> DateTimeFormat<'d> {
w: &mut impl std::fmt::Write,
value: &impl DateTimeInput,
) -> std::fmt::Result {
datetime::write_pattern(&self.pattern, self.symbols.get(), value, &self.locale, w)
.map_err(|_| std::fmt::Error)
datetime::write_pattern(
&self.pattern,
self.symbols.as_ref().map(|s| s.get()),
value,
&self.locale,
w,
)
.map_err(|_| std::fmt::Error)
}

/// Takes a [`DateTimeInput`] implementer and returns it formatted as a string.
Expand Down
100 changes: 74 additions & 26 deletions components/datetime/src/format/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use crate::arithmetic;
use crate::date::{DateTimeInput, DateTimeInputWithLocale, LocalizedDateTimeInput};
use crate::error::DateTimeFormatError as Error;
use crate::fields::{self, FieldLength, FieldSymbol};
use crate::fields::{self, Field, FieldLength, FieldSymbol};
use crate::pattern::{Pattern, PatternItem};
use crate::provider;
use crate::provider::helpers::DateTimeSymbols;
Expand Down Expand Up @@ -46,7 +46,7 @@ where
T: DateTimeInput,
{
pub(crate) pattern: &'l Pattern,
pub(crate) symbols: &'l provider::gregory::DateSymbolsV1,
pub(crate) symbols: Option<&'l provider::gregory::DateSymbolsV1>,
pub(crate) datetime: &'l T,
pub(crate) locale: &'l Locale,
}
Expand Down Expand Up @@ -95,7 +95,7 @@ where

pub fn write_pattern<T, W>(
pattern: &crate::pattern::Pattern,
symbols: &provider::gregory::DateSymbolsV1,
symbols: Option<&provider::gregory::DateSymbolsV1>,
datetime: &T,
locale: &Locale,
w: &mut W,
Expand All @@ -114,10 +114,15 @@ where
Ok(())
}

// This function assumes that the correct decision has been
// made regarding availability of symbols in the caller.
//
// When modifying the list of fields using symbols,
// update the matching query in `analyze_pattern` function.
pub(super) fn write_field<T, W>(
pattern: &crate::pattern::Pattern,
field: &fields::Field,
symbols: &crate::provider::gregory::DateSymbolsV1,
symbols: Option<&crate::provider::gregory::DateSymbolsV1>,
datetime: &impl LocalizedDateTimeInput<T>,
w: &mut W,
) -> Result<(), Error>
Expand Down Expand Up @@ -146,16 +151,18 @@ where
field.length,
)?,
length => {
let symbol = symbols.get_symbol_for_month(
month,
length,
datetime
.datetime()
.month()
.ok_or(Error::MissingInputField)?
.number as usize
- 1,
);
let symbol = symbols
.expect("Expect symbols to be present")
.get_symbol_for_month(
month,
length,
datetime
.datetime()
.month()
.ok_or(Error::MissingInputField)?
.number as usize
- 1,
);
w.write_str(symbol)?
}
},
Expand All @@ -164,7 +171,9 @@ where
.datetime()
.iso_weekday()
.ok_or(Error::MissingInputField)?;
let symbol = symbols.get_symbol_for_weekday(weekday, field.length, dow);
let symbol = symbols
.expect("Expect symbols to be present")
.get_symbol_for_weekday(weekday, field.length, dow);
w.write_str(symbol)?
}
FieldSymbol::Day(..) => format_number(
Expand Down Expand Up @@ -221,23 +230,62 @@ where
field.length,
)?,
FieldSymbol::DayPeriod(period) => {
let symbol = symbols.get_symbol_for_day_period(
period,
field.length,
datetime.datetime().hour().ok_or(Error::MissingInputField)?,
arithmetic::is_top_of_hour(
pattern,
datetime.datetime().minute().map(u8::from).unwrap_or(0),
datetime.datetime().second().map(u8::from).unwrap_or(0),
),
);
let symbol = symbols
.expect("Expect symbols to be present")
.get_symbol_for_day_period(
period,
field.length,
datetime.datetime().hour().ok_or(Error::MissingInputField)?,
arithmetic::is_top_of_hour(
pattern,
datetime.datetime().minute().map(u8::from).unwrap_or(0),
datetime.datetime().second().map(u8::from).unwrap_or(0),
),
);
w.write_str(symbol)?
}
field @ FieldSymbol::TimeZone(_) => return Err(Error::UnsupportedField(field)),
};
Ok(())
}

// This function determins whether the struct will load symbols data.
// Keep it in sync with the `write_field` use of symbols.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: "Keep it in sync" is a noble comment, but the reality is that devs don't obey instructions in code comments unless there is a hard error. Is there a way to make this into more of a hard error? I guess test coverage, to make sure we hit your .expect(), but is that all we can do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it is. I could try to build some more elaborate enum that has to be true here and in the analyze function together, but I think it's overengineering at this point, and I'd prefer to land it as is.

pub fn analyze_pattern(pattern: &Pattern, supports_time_zones: bool) -> Result<bool, &Field> {
let fields = pattern.items().iter().filter_map(|p| match p {
PatternItem::Field(field) => Some(field),
_ => None,
});

let mut requires_symbols = false;

for field in fields {
if !requires_symbols {
requires_symbols = match field.symbol {
FieldSymbol::Month(_) => {
!matches!(field.length, FieldLength::One | FieldLength::TwoDigit)
}
FieldSymbol::Weekday(_) | FieldSymbol::DayPeriod(_) => true,
_ => false,
}
sffc marked this conversation as resolved.
Show resolved Hide resolved
}

if supports_time_zones {
if requires_symbols {
// If we require time zones, and symbols, we know all
// we need to return already.
break;
}
} else if matches!(field.symbol, FieldSymbol::TimeZone(_)) {
// If we don't support time zones, and encountered a time zone
// field, error out.
return Err(field);
}
Comment on lines +273 to +283
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I don't see the point of this if/elseif. More specifically, I don't think checking supports_time_zones affects any behavior. I think you can remove the supports_time_zones argument and just unconditionally break whenever requires_symbols gets set (or, better, delete that variable and just return true).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is that we also validate the time zone availability here. If I were to apply the change you're suggesting, assuming I understand it, then we wouldn't error out if we established we need symbols but also the pattern used time zone and was called from non-time-zoned DTF.

Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I didn't catch that you actually wanted to validate something. This is fine then.

}

Ok(requires_symbols)
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -268,7 +316,7 @@ mod tests {
let mut sink = String::new();
write_pattern(
&pattern,
&data.get(),
Some(&data.get()),
&datetime,
&"und".parse().unwrap(),
&mut sink,
Expand Down
6 changes: 5 additions & 1 deletion components/datetime/src/format/zoned_datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ where
W: fmt::Write + ?Sized,
{
let pattern = &zoned_datetime_format.datetime_format.pattern;
let symbols = zoned_datetime_format.datetime_format.symbols.get();
let symbols = zoned_datetime_format
.datetime_format
.symbols
.as_ref()
.map(|s| s.get());

match field.symbol {
FieldSymbol::TimeZone(_time_zone) => time_zone::write_field(
Expand Down
42 changes: 25 additions & 17 deletions components/datetime/src/zoned_datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ use icu_provider::{DataProvider, DataRequest, ResourceOptions, ResourcePath};
use crate::{
date::ZonedDateTimeInput,
datetime::DateTimeFormat,
format::zoned_datetime::{self, FormattedZonedDateTime},
format::{
datetime,
zoned_datetime::{self, FormattedZonedDateTime},
},
options::DateTimeFormatOptions,
provider::{self, helpers::DateTimePatterns},
time_zone::TimeZoneFormat,
Expand Down Expand Up @@ -124,27 +127,32 @@ impl<'d> ZonedDateTimeFormat<'d> {
})?
.take_payload()?;

let symbols_data: icu_provider::DataPayload<
'_,
'_,
provider::gregory::DateSymbolsV1Marker,
> = date_provider
.load_payload(&DataRequest {
resource_path: ResourcePath {
key: provider::key::GREGORY_DATE_SYMBOLS_V1,
options: ResourceOptions {
variant: None,
langid: Some(locale.clone().into()),
},
},
})?
.take_payload()?;

let pattern = pattern_data
.get()
.get_pattern_for_options(options)?
.unwrap_or_default();

let requires_data = datetime::analyze_pattern(&pattern, true)
.map_err(|field| DateTimeFormatError::UnsupportedField(field.symbol))?;

let symbols_data = if requires_data {
Some(
date_provider
.load_payload(&DataRequest {
resource_path: ResourcePath {
key: provider::key::GREGORY_DATE_SYMBOLS_V1,
options: ResourceOptions {
variant: None,
langid: Some(locale.clone().into()),
},
},
})?
.take_payload()?,
)
} else {
None
};

let datetime_format = DateTimeFormat::new(locale, pattern, symbols_data);
let time_zone_format = TimeZoneFormat::try_new(
datetime_format.locale.clone(),
Expand Down