Remove generic CldrCalendar impl for Hijri and add update docs #7005

sffc · 2025-09-30T19:03:40Z

Users with a custom Hijri should implement CldrCalendar themselves. I don't want to commit to a single immutable set of patterns and display names for eternity.

sffc · 2025-09-30T20:22:53Z

components/datetime/src/scaffold/calendar.rs

+impl CldrCalendar for Hijri<hijri::UmmAlQura> {
+    type YearNamesV1 = DatetimeNamesYearHijriV1;
+    type MonthNamesV1 = DatetimeNamesMonthHijriV1;
+    type SkeletaV1 = DatetimePatternsDateHijriV1;
+}


Note: This trait is very easy to implement in userland, easier than in 1.0. All you need to do is look at the menu of available data markers and pick the ones you want. If you have special needs, you can even mix and match stock data markers and custom data markers.

nothing that involves the word data marker is easy for users in my experience

robertbastian · 2025-09-30T20:23:50Z

components/datetime/src/scaffold/calendar.rs

+impl CldrCalendar for Hijri<hijri::UmmAlQura> {
+    type YearNamesV1 = DatetimeNamesYearHijriV1;
+    type MonthNamesV1 = DatetimeNamesMonthHijriV1;
+    type SkeletaV1 = DatetimePatternsDateHijriV1;
+}


nothing that involves the word data marker is easy for users in my experience

components/calendar/src/types.rs

robertbastian · 2025-09-30T20:26:24Z

components/calendar/src/types.rs

 /// Year information for a year that is specified as a cyclic year
+///
+/// Knowing the cyclic year is typically not enough to pinpoint a date, however cyclic calendars
+/// don't typically use eras, so disambiguation can be done by saying things like "Year 甲辰 (2024)"


this is an out-struct, so it doesn't need to "pinpoint" a date

I also don't like the phrasing of "disambiguation can be done by saying things like"

These are pre-existing docs that I moved. They were previously on YearInfo::Cyclic.

ok but this is an "update docs" PR so let's update them

The cyclic year is insufficient to fully specify a date, and cyclic calendars typically do not use eras. In cases where a date needs to be unambiguously specified, one can do things like (...) using the related_iso field.

The month is also insufficient to fully specify a date, yet we don't put a disclaimer that you need to add a different field if you're doing formatting.

This type contains the information to fully specify a year (it has a related_iso field).

What the comment is trying to explain is how to display this type "in cases where a date needs to be unambiguously specified", which I really don't think should be explained in this crate, as these cases are very locale-dependent. We also don't explain how EraYear should be displayed.

I think it's good context as to why there's a related_iso field

Then move this to the field? Link to other explanations like we do for extended year?

components/calendar/src/types.rs

robertbastian · 2025-09-30T20:29:34Z

components/datetime/src/provider/neo/mod.rs

    /// This calendar has a small, fixed set of eras with numeric years, this stores the era names in chronological order.
    ///
-    /// See FormattableEra for a definition of what chronological order is in this context.
+    /// See [`EraYear`](icu_calendar::types::EraYear) for a definition of what chronological order is in this context.


EraYear does not define a "chronological order". You probably want to point to the era_index field, which is definitely not chronological

robertbastian · 2025-09-30T20:31:07Z

components/datetime/src/scaffold/calendar.rs

+/// - The era and year names are loaded from [`Self::YearNamesV1`] and accessed based on [`YearInfo`]
+/// - The month name is loaded from [`Self::MonthNamesV1`] and accessed based on [`MonthInfo`]


this is really internal, I'd rather not have users deal with it

This is the public interface between YearInfo/YearNames and MonthInfo/MonthNames. It ought to be documented. Users need to worry about it only if they're implementing a custom calendar.

You say public interface, I say unstable provider code.

Manishearth · 2025-09-30T20:51:23Z

Also, for reference, the justification behind not unifying the formatting is given in #6963 (comment), under "Hijri formatting". Specifically, CLDR may choose to have divergent formatting for these calendars.

When discussing that, I was unconvinced of arguments like "umm al-qura may mandate a different month name" (that, to me, is a regional concern), however there is a good argument to be made for regions with multiple calendars tacking disambiguators on their format strings, e.g. something like Ramadan 5, 1447 [Umm al-Qura].

Also, either way I think we should not unify formatting unless CLDR commits to the formatting being the same.

Too bad we don't have specialization; I'd have been very comfortable with unified formatting under specialization.

Manishearth

(Generally in favor of this approach, at least for 2.1)

Co-authored-by: Robert Bastian <[email protected]>

sffc · 2025-09-30T21:14:13Z

Too bad we don't have specialization; I'd have been very comfortable with unified formatting under specialization.

Yes, I'm totally okay with having a default formatting impl, but it must be overridable. That's non-negotiable

sffc · 2025-09-30T21:15:31Z

I would not object to exporting a macro like impl_formatting_for_hijri! that implements the trait if @robertbastian thinks the trait is too internal, but I think the trait shouldn't be seen as "too internal".

robertbastian

Users with a custom Hijri should implement CldrCalendar themselves

It is not actually possible to implement CldrCalendar for Hijri<CustomRules> due to the orphan rule. You'd need a newtype for which you'd have to implement Calendar as well. It's a lot of boilerplate and unstable code.

I'm fine with reverting this for now and just not offering this functionality, but I would still like to have the generic implementation in the future.

there is a good argument to be made for regions with multiple calendars tacking disambiguators on their format strings, e.g. something like Ramadan 5, 1447 [Umm al-Qura].

Such disambiguators will not necessarily be in the skeleton. Another solution would be to introduce a new calendar field, or put them in the era name.

robertbastian · 2025-10-02T12:16:29Z

components/datetime/src/provider/neo/mod.rs

-    /// This calendar has a small, fixed set of eras with numeric years, this stores the era names in chronological order.
-    ///
-    /// See [`EraYear`](icu_calendar::types::EraYear) for a definition of what chronological order is in this context.
+    /// This calendar has a small, fixed set of eras with numeric years. Eras are stored


keep the first sentence of a doc comment as its own paragraph.

robertbastian · 2025-10-02T17:10:44Z

I think we should introduce a new trait in icu_datetime: FormattableHijriRules: HijriRules. That clients can implement on their custom rules, and we can implement it on the rules from icu_calendar. Then we have a generic implementation for Hijri<R: FormattableHijriRules>.

It would be nice to have default associated types on that, so clients can just say impl FormattableHijriRules for MyRules {} but rust-lang/rust#29661

sffc · 2025-10-02T18:38:16Z

It is not actually possible to implement CldrCalendar for Hijri<CustomRules> due to the orphan rule.

Good point

I think we should introduce a new trait in icu_datetime: FormattableHijriRules: HijriRules

Yep, I think this direction is one we should explore. It's an example of why I don't want to rush into shipping these things.

sffc · 2025-10-02T21:17:30Z

This is mostly replaced by #7028 but there are some docs that might be worth resurrecting.

#7005

sffc added 2 commits September 30, 2025 11:58

Don't blanket-impl CldrCalendar for Hijri

3bb5417

Change CldrCalendar docs to be unstable-but-implementable

e939d12

sffc requested review from Manishearth and robertbastian September 30, 2025 20:05

sffc marked this pull request as ready for review September 30, 2025 20:05

sffc requested a review from zbraniecki as a code owner September 30, 2025 20:05

sffc added 2 commits September 30, 2025 13:09

one more hijri::Rules in icu_datetime

89efadf

More docs improvements

daf0da8

sffc commented Sep 30, 2025

View reviewed changes

robertbastian requested changes Sep 30, 2025

View reviewed changes

Manishearth reviewed Sep 30, 2025

View reviewed changes

Apply suggestions from code review

6486d68

Co-authored-by: Robert Bastian <[email protected]>

sffc requested a review from robertbastian September 30, 2025 21:11

sffc added 2 commits September 30, 2025 14:11

Feedback

c28484f

doc

2268368

robertbastian reviewed Oct 2, 2025

View reviewed changes

robertbastian mentioned this pull request Oct 2, 2025

Allow different Hijri calendars to use different names #7028

Merged

sffc marked this pull request as draft October 2, 2025 21:17

robertbastian added a commit that referenced this pull request Oct 2, 2025

Allow different Hijri calendars to use different names (#7028)

f73ca15

#7005

		/// - The era and year names are loaded from [`Self::YearNamesV1`] and accessed based on [`YearInfo`]
		/// - The month name is loaded from [`Self::MonthNamesV1`] and accessed based on [`MonthInfo`]

Remove generic CldrCalendar impl for Hijri and add update docs #7005

Are you sure you want to change the base?

Remove generic CldrCalendar impl for Hijri and add update docs #7005

Uh oh!

Conversation

sffc commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Sep 30, 2025

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Sep 30, 2025

Uh oh!

sffc commented Sep 30, 2025

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian commented Oct 2, 2025

Uh oh!

sffc commented Oct 2, 2025

Uh oh!

sffc commented Oct 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sffc commented Sep 30, 2025 •

edited

Loading

robertbastian Oct 2, 2025 •

edited

Loading