Add Date, Time, and DateTime from str impls via IXDTF #5260
Conversation
sffc
requested review from
zbraniecki,
nordzilla,
Manishearth and
a team
as code owners
sffc
requested review from
nekevss and
robertbastian
and removed request for
a team,
zbraniecki,
Manishearth and
nordzilla
components/calendar/src/ixdtf.rs
Outdated
| impl TryFrom<DateRecord> for Date<Iso> { | ||
| type Error = RangeError; | ||
| fn try_from(value: DateRecord) -> Result<Self, Self::Error> { | ||
| Self::try_new_iso_date(value.year, value.month, value.day) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<TimeRecord> for Time { | ||
| type Error = RangeError; | ||
| fn try_from(value: TimeRecord) -> Result<Self, Self::Error> { | ||
| Self::try_new(value.hour, value.minute, value.second, value.nanosecond) | ||
| } | ||
| } | ||
|
|
||
| impl AnyCalendar { | ||
| #[cfg(feature = "compiled_data")] | ||
| fn try_from_ixdtf_record(ixdtf_record: &IxdtfParseRecord) -> Result<Self, FromIxdtfError> { | ||
| let calendar_id = ixdtf_record.calendar.unwrap_or(b"iso"); | ||
| let calendar_kind = crate::AnyCalendarKind::get_for_bcp47_bytes(calendar_id) | ||
| .ok_or(FromIxdtfError::UnknownCalendar)?; | ||
| let calendar = AnyCalendar::new(calendar_kind); | ||
| Ok(calendar) | ||
| } | ||
| } |
There was a problem hiding this comment.
hmm the records feel like ixdtf parser implementation types, so I'd rather not expose conversions from them. Date and Time are trivial enough to be done manually, and for AnyCalendar I don't really see the use case
There was a problem hiding this comment.
I figured since DateRecord and TimeRecord are public types, we should have public conversions. But, actually, it puts ixdtf into icu_datetime's API, so we should probably at least wait until ixdtf has a 1.0 release.
Co-authored-by: Robert Bastian <[email protected]>
| @@ -12,7 +12,7 @@ use ixdtf::ParserError; | |||
| /// An error returned from parsing an IXDTF string to an `icu_calendar` type. | |||
| #[derive(Debug)] | |||
| #[non_exhaustive] | |||
| pub enum FromStrError { | |||
| pub enum ParseError { | |||
| /// Syntax error in the IXDTF string. | |||
| Syntax(ParserError), | |||
There was a problem hiding this comment.
follow-up: this should also be ParseError instead of ParserError.
| #[cfg(any( | ||
| feature = "icu_datetime", | ||
| feature = "icu_timezone", | ||
| feature = "icu_calendar" | ||
| ))] | ||
| pub enum CalendarFromStrError { | ||
| pub enum CalendarParseError { |
There was a problem hiding this comment.
I get how this name came to be, but it's still weird because we're not parsing calendars. Actually the CalendarError above should probably be called DateTimeError, as you get this when constructing a datetime fails. You get this one when parsing a datetime or date or time fails, so I think DateTimeParseError would be better. But this can be a follow-up to fix the other name as well.
There was a problem hiding this comment.
This all comes back to the icu_datetime vs icu_calendar crate name. The icu_datetime crate is formatting for datetimes, so that is the correct name for it. The icu_calendar crate's primary value proposition, and the vast majority of code it contains, involves non-Gregorian calendars. It also happens to export the Date and Time types that we use in icu_datetime for formatting.
By our current naming conventions, DateTimeParseError would be an error originating from parsing a string in the icu_datetime crate, but that is not what this error is.
icu_calendar is more of a utility crate, except it's not, because it depends heavily on CLDR data and algorithms. But maybe if we go with cldr_pattern and/or cldr_unit, we could rename icu_calendar to cldr_datetime or something like that.
There was a problem hiding this comment.
By our current naming conventions, DateTimeParseError would be an error originating from parsing a string in the icu_datetime crate, but that is not what this error is.
Errors are not prefixed by crates anymore, errors are named for the operations where they originate.
There was a problem hiding this comment.
Oh?
LocaleParseError==icu::locale::ParseErrorFixedDecimalParseError==fixed_decimal::ParseErrorFixedDecimalLimitError==fixed_decimal::LimitErrorTimeZoneInvalidOffsetError==icu::timezone::InvalidOffsetError
...
There was a problem hiding this comment.
LocaleParseError==icu4x::Locale::from_string's errorFixedDecimalParseError==icu4x::FixedDecimal::from_string's errorFixedDecimalLimitError==icu4x::FixedDecimal::from_double's errorTimeZoneInvalidOffsetError==icu4x::CustomTimeZone::try_set_gmt_offset_seconds's error
...
|
wtf this PR was green just 10 minutes ago, now I come back and it has conflicts already? I already resolved conflicts like 2 other times today. |
#2127
#5201