-
Notifications
You must be signed in to change notification settings - Fork 407
CLDR-18464 Add a Before Hijrah era to the Islamic calendars #4581
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.
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
Conversation
<calendar type="islamic"> | ||
<calendarSystem type="lunar" /> | ||
<eras> | ||
<era type="-1" end="622-7-14" code="islamic-inverse" aliases="bh"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it the code "islamic-inverse" because I want to land the code updates, #4519, atomically once we have agreement on them.
<calendar type="islamic-civil"> | ||
<calendarSystem type="lunar" /> | ||
<eras> | ||
<era type="-1" end="622-7-15" code="islamic-civil-inverse" aliases="bh"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it the type -1 because type 0 is already used for the AH era. Please let me know if I should instead:
- Use a positive number like 1 (and leave AH as 0)
- Change BH to 0, AH to 1, and update all the XML files that currently have translations for AH at type 0 to instead be at type 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me. I would also prefer the "Change BH to 0, AH to 1" option, as it seems cleanest.
This means changing all the XML files, which I'm willing to do and can probably automate, but to make sure, before I proceed, is it sufficient for me to change something like <eraAbbr>
<era type="0">যুগ</era>
</eraAbbr> to <eraAbbr>
<era type="1">যুগ</era>
</eraAbbr> without adding structure for type="0"? It will inherit automatically, right? |
As far as I know, yes. |
Does this look right? Here's how I generated the diff: $ xmlstarlet ed --inplace -P -u "/ldml/dates/calendars/calendar[@type='islamic']/eras/*/era[@type=0]/@type" -v 1 *.xml That updated all the XML files, but it also resulted in some other unwanted diffs to the XML. I undid those by manually touching up the diff file to remove diffs on lines containing To verify that I did everything, correct, I then ran: $ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic']/eras/*/era[@type=0]" *.xml
Warning: program compiled against libxml 212 using older 209
Before Hijrah
BH
BH
$ So it found the three BH entries I put in and nothing else. |
The other calendars don't have data. Only $ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-civil']/eras/*/era[@type=0]" *.xml
$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-rgsa']/eras/*/era[@type=0]" *.xml
$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-umalqura']/eras/*/era[@type=0]" *.xml
$ xmlstarlet sel -t -v "/ldml/dates/calendars/calendar[@type='islamic-tbla']/eras/*/era[@type=0]" *.xml
$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs commit fix
@srl295, do you prefer me to merge the PR in this shape (AH becomes index 1, BH inserted at index 0) or would you prefer keeping AH at index 0 and adding BH at index 1 as @FrankYFTang suggested? |
@sffc I am not @srl295 but I now prefer keeping AH at index 0 and adding BH at index 1 |
OK please re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall PR is straightforward to review now, with changes in other locales undone
Do we need a followup issue to make sure CLDR spec clarifies this question so that it doesn't need to get discussed again? Variants and/or lack thereof? |
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
This is ready to merge; should I Squash And Merge or Rebase And Merge? I'd like to use CLDR-18369 for all spec updates on this topic. |
We usually Squash and merge except for special circumstances such as cherry-picking maint branch changes to main. But since this only has 1 commit does it make a difference? Does squash gives a different commit hash? |
This causes an IndexOutOfBoundsException at org.unicode.cldr.test.ExampleGenerator.handleEras(ExampleGenerator.java:3158), because there is a reference to a hard-coded CALENDAR_ERAS HashMap maps "islamic" to a sample date for only a single era, need to add another entry for the new islamic era. I will make a PR to do that. |
CLDR-18464
ALLOW_MANY_COMMITS=true
CC @roozbehp @pedberg-icu @AEApple