Add Iran's Hijri implementation #6474
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
ebraminio
force-pushed
the
main
branch
6 times, most recently
from
7f31be8 to
5e76726
Compare
ebraminio
requested review from
zbraniecki,
nciric,
Manishearth and
sffc
as code owners
ebraminio
force-pushed
the
main
branch
2 times, most recently
from
031dac8 to
f710466
Compare
|
Ooh, thank you for doing this work! I'll try and have a look when I have time. We may want to keep it out of the default anycalendar set for now. |
Manishearth
reviewed
Apr 21, 2025
There was a problem hiding this comment.
Overall the implementation looks good.
Some general questions for @robertbastian and @sffc (background: this is a data-driven calendar that falls back to tabular islamic)
- In general we seem to be fine with having non-Any calendar implementations to this crate. I assume we're okay with adding this one? Naming thoughts?
- We should figure out criteria by which this gets added to Any. We'll need a BCP47 tag probably. And some agreement about shipping the data by default.
- Code structure wise: should this data be living in calendrical_calculations?
- Code structure wise: should we clean up the hijri module? It's getting big.
I'm going to suggest we hold off on merging this until 2.0, though I'm happy to have it merged as a private implementation til then.
|
Agreed that we should start by landing this in the same way as JapaneseExtended and HijriSimulated (available in icu_calendar but not included in DateTimeFormatter, but potentially formattable with FixedCalendarDateTimeFormatter). Also @hsivonen. |
Manishearth
reviewed
Apr 24, 2025
components/locale_core/src/preferences/extensions/unicode/keywords/calendar.rs
Outdated
Show resolved
Hide resolved
Manishearth
added a commit
that referenced
this pull request
Apr 30, 2025
As #6474 (comment) --------- Co-authored-by: Manish Goregaokar <[email protected]>
ebraminio
added a commit
to ebraminio/icu4x
that referenced
this pull request
Apr 30, 2025
ebraminio
added a commit
to ebraminio/icu4x
that referenced
this pull request
Apr 30, 2025
ebraminio
added a commit
to ebraminio/icu4x
that referenced
this pull request
Apr 30, 2025
ebraminio
force-pushed
the
main
branch
2 times, most recently
from
8d781b8 to
ff7d362
Compare
ebraminio
added a commit
to ebraminio/icu4x
that referenced
this pull request
May 6, 2025
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This tries to implement what historically till present observed in Iran with Islamic calendar. This uses the data gathered in https://github.com/roozbehp/qamari
Related to #6336