DTF calendar handling #6350
DTF calendar handling #6350
Conversation
robertbastian
force-pushed
the
cals
branch
2 times, most recently
from
94ab4e8 to
e7f9c0f
Compare
robertbastian
force-pushed
the
cals
branch
3 times, most recently
from
010a326 to
cb96117
Compare
| _ => prefs.calendar_algorithm = None, | ||
| } | ||
| let kind = AnyCalendarKind::new((&prefs).into()); | ||
| // AnyCalendarKind constructor should not select an unsupported fallback |
There was a problem hiding this comment.
this is unfortunate
There was a problem hiding this comment.
Yeah, a way to fix this would be for icu_calendar to export an enum LocaleDefaultCalendarKind which we can exhaustively match over here. But this is fine. I do prefer for this to be a debug_assert instead of an assert.
robertbastian
requested review from
sffc,
Manishearth,
zbraniecki and
a team
as code owners
| _ => prefs.calendar_algorithm = None, | ||
| } | ||
| let kind = AnyCalendarKind::new((&prefs).into()); | ||
| // AnyCalendarKind constructor should not select an unsupported fallback |
There was a problem hiding this comment.
Yeah, a way to fix this would be for icu_calendar to export an enum LocaleDefaultCalendarKind which we can exhaustively match over here. But this is fine. I do prefer for this to be a debug_assert instead of an assert.
| DataError::custom("Don't know how to load data for specified calendar") | ||
| .with_debug_context(&self.kind), | ||
| ), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
Issue: I won't approve this with a panic. It is not a panic that the compiler can easily eliminate.
| kind: AnyCalendarKind, | ||
| _helper: PhantomData<H>, | ||
| } | ||
|
|
||
| impl<H, P> AnyCalendarProvider<H, P> { | ||
| pub(crate) fn new(provider: P, kind: AnyCalendarKind) -> Self { | ||
| pub(crate) fn new_unchecked(provider: P, kind: AnyCalendarKind) -> Self { |
There was a problem hiding this comment.
Suggestion: you could make this try_new -> Option<Self> and do the match in here. Less coding at a distance. I don't really like having a match in the middle of neo.rs which is supposed to be a thin API wrapper file.
There was a problem hiding this comment.
I did this in a different way in #6352
| AnyCalendarKind::Buddhist | ||
| } else if region == Some(region!("AF")) || region == Some(region!("IR")) { | ||
| AnyCalendarKind::Persian | ||
| } else if region == Some(region!("SA")) && algo == Some(CalendarAlgorithm::Hijri(None)) { |
There was a problem hiding this comment.
thought: still unhappy with us treating -u-ca-islamic as a "none islamic" calendar; we don't do that for any other category of calendar, and it's mostly a meaningless code
But it's fine for now, we also need ICU4C compatability.
| | CalendarAlgorithm::Ethiopic | ||
| | CalendarAlgorithm::Gregory | ||
| | CalendarAlgorithm::Hebrew | ||
| | CalendarAlgorithm::Hijri(_) |
There was a problem hiding this comment.
thought: oh, huh, because the formatter doesn't distinguish between islamic calendars we don't actually need to selectively support specific ones. clever.
| @@ -591,6 +591,8 @@ pub struct FixedCalendarDateTimeNames<C, FSet: DateTimeNamesMarker = CompositeDa | |||
| #[derive(Debug, Clone)] | |||
| pub struct DateTimeNames<FSet: DateTimeNamesMarker> { | |||
| inner: FixedCalendarDateTimeNames<(), FSet>, | |||
| // calendar.kind() is one of Buddhist, Chinese, Coptic, Dangi, Ethiopian, EthiopianAmeteAlem, Gregorian, Hebrew, Indian, | |||
There was a problem hiding this comment.
suggestion (nb): should we also have a method on CalendarKind called is_core_supported() or something like that?
There was a problem hiding this comment.
I specifically don't want icu_calendar to define any sets for icu_datetime
|
I made a new PR incorporating this one: #6352 |
Fixes #6227 Toward #4889 Replaces #6350 --------- Co-authored-by: Robert Bastian <[email protected]>
No description provided.