Skip to content

Conversation

sffc
Copy link
Member

@sffc sffc commented Apr 17, 2025

CLDR-18465

  • This PR completes the ticket.

Related to #4581. Wanted to get feedback on how to remove an era.

@pedberg-icu @srl295

ALLOW_MANY_COMMITS=true

@sffc sffc changed the title CLDR-18465 Remove Before Diocletian are from Coptic calendar CLDR-18465 Remove Before Diocletian era from Coptic calendar Apr 17, 2025
@sffc sffc requested review from srl295 and pedberg-icu April 18, 2025 19:02
srl295
srl295 previously approved these changes Apr 21, 2025
@sffc
Copy link
Member Author

sffc commented Apr 21, 2025

Please note that I did not go through and delete all of the existing BD data. Should I?

@sffc sffc requested a review from roozbehp April 22, 2025 05:52
@sffc
Copy link
Member Author

sffc commented Apr 24, 2025

What are the next steps on this PR?

@sffc
Copy link
Member Author

sffc commented May 1, 2025

Hi, can I have a review on this PR?

@macchiati
Copy link
Member

For normal tickets (eg non-infrastructure) the process is to get them accepted in the next TC meeting, to make sure that there is visibility into substantive changes. So can you add this to next week's CLDR agenda?

(and btw, the tests are failing.)

@sffc
Copy link
Member Author

sffc commented May 23, 2025

The Design WG said on 2025-05-05:

Recommendation: 2.b.ii +
Keep type values and codes stable in CLDR; same meaning across time. 
No gaps, no overlaps in datetime span.
Abandon zero-based and ascending order: don’t care about gaps or order; numbers act like string keys.
In formatting, years before first (in time) are negative, years after end just extend.

2.b.ii says

Add an era type key in locale display name data – a string key, forming a map=dict, not just an array

I don't actually know what that part means.

So what is the intended implementation of this PR? Is it to delete all data for era 0 and leave the data for era 1 as-is, or is it something more than that?

@sffc
Copy link
Member Author

sffc commented Jun 4, 2025

Hi, can I get a signal on how I should proceed with this PR?

@macchiati @pedberg-icu @roozbehp @FrankYFTang

@sffc
Copy link
Member Author

sffc commented Jul 23, 2025

Please, I want to land this and its dependent PRs in CLDR 48 but I've been blocked for months on reviewer feedback.

@robertbastian
Copy link
Member

is this still targeted for v48?

@srl295
Copy link
Member

srl295 commented Aug 26, 2025

Please, I want to land this and its dependent PRs in CLDR 48 but I've been blocked for months on reviewer feedback.

It's also still failing tests

@sffc
Copy link
Member Author

sffc commented Aug 27, 2025

I'm holding off on merging this until the ICU change sticks (ICU-23172 which apparently has memory leaks), but I would still like to see it in CLDR 48.

@sffc
Copy link
Member Author

sffc commented Aug 27, 2025

The test failures say

2025-08-27T00:48:28.7632457Z     TestAllLocales
2025-08-27T00:48:28.7636883Z ##[error] (TestCheckCLDR.java:503)  Error: : bs //ldml/dates/calendars/calendar[@type="coptic"]/eras/eraAbbr/era[@type="0"]: expected != null

@srl295, can you suggest how to go about fixing that?

@pedberg-icu
Copy link
Contributor

pedberg-icu commented Aug 28, 2025

I'm holding off on merging this until the ICU change sticks (ICU-23172 which apparently has memory leaks), but I would still like to see it in CLDR 48.

@sffc Those memory leaks were fixed, along with an inheritance issue for narrow eras that should have inherited from root, and the PR was merged. Many of the memory leaks were actually in logKnownIssue test skips, nothing to do with the eraNames stuff.

@AEApple AEApple closed this Sep 4, 2025
@AEApple AEApple reopened this Sep 4, 2025
@AEApple
Copy link
Contributor

AEApple commented Sep 4, 2025

Test is here:

assertNotNull(locale + " " + path, resolved.getStringValue(path));

Looks like there are inherited values in bs.xml for example:

<era type="0">↑↑↑</era>

@sffc - maybe you need to remove all the values for that era across all of the locales files? There is a CLDR tool if it's helpful: https://cldr.unicode.org/development/cldr-big-red-switch/cldrmodify-using-config-file

@sffc
Copy link
Member Author

sffc commented Sep 8, 2025

I'm trying but running into multiple roadblocks in trying to run the CLDRModify tool. I booked some time tomorrow to work through it with you.

@sffc
Copy link
Member Author

sffc commented Sep 8, 2025

Thanks @macchiati and @AEApple for your help with the CLDRModify tool. I've updated the PR.

@AEApple
Copy link
Contributor

AEApple commented Sep 8, 2025

@sffc - Can you include the CLDR modify config file in the PR? We like including them so we have a history of the CLDR modify command that was run to generate the data changes. Thanks!

@sffc
Copy link
Member Author

sffc commented Sep 8, 2025

Still some test failures:

  TestCheckCLDR {
    Test14866
 (0.137s) Passed
    TestA (0.720s) Passed
    TestALLOWED_IN_LIMITED_PATHS (0.000s) Passed
    TestAllLocales
Error:  (TestCheckCLDR.java:503)  Error: : bs //ldml/dates/calendars/calendar[@type="coptic"]/eras/eraAbbr/era[@type="0"]: expected != null

Error:  (TestCheckCLDR.java:503)  Error: : bs //ldml/dates/calendars/calendar[@type="coptic"]/eras/eraNarrow/era[@type="0"]: expected != null

Error:  (TestCheckCLDR.java:503)  Error: : cs //ldml/dates/calendars/calendar[@type="coptic"]/eras/eraAbbr/era[@type="0"]: expected != null

Error:  (TestCheckCLDR.java:503)  Error: : cs //ldml/dates/calendars/calendar[@type="coptic"]/eras/eraNarrow/era[@type="0"]: expected != null

...

@AEApple
Copy link
Contributor

AEApple commented Sep 8, 2025

It looks like it's still failing since you didn't do the following two paths for all locales:

//ldml/dates/calendars/calendar[@type="coptic"]/eras/eraAbbr/era[@type="0"]
//ldml/dates/calendars/calendar[@type="coptic"]/eras/eraNarrow/era[@type="0"]

You'll need to make sure all are removed.

@sffc
Copy link
Member Author

sffc commented Sep 9, 2025

Yay, tests are passing!

@AEApple
Copy link
Contributor

AEApple commented Sep 9, 2025

So the CLDRModify change LGTM, but I don't know if there is any impact with re-ordering the calendars by calendar type name in English. I assume it's probably okay, but someone who knows how the calendars work should be the one to approve just in case there's some consideration that I don't know about.

@sffc
Copy link
Member Author

sffc commented Sep 9, 2025

It re-ordered XML that I had previously added in this same PR. The PR only adds and deletes. It does not move.

Copy link
Contributor

@AEApple AEApple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! You will need to squash to fix the commit before you can merge.

@macchiati
Copy link
Member

macchiati commented Sep 9, 2025 via email

@macchiati
Copy link
Member

@sffc Is this done? It looks like the only problem is the jira ticket, so you need to squash. Can you take care of that so we can mark the ticket as done (we're trying to clean up all the alpha integration tickets.

@sffc sffc force-pushed the CLDR-18465-coptic branch from 96d4b44 to a3ca132 Compare September 9, 2025 19:44
@sffc
Copy link
Member Author

sffc commented Sep 9, 2025

Squashed

@sffc sffc merged commit 4462952 into unicode-org:main Sep 9, 2025
11 checks passed
@sffc sffc deleted the CLDR-18465-coptic branch September 9, 2025 21:07
@robertbastian robertbastian mentioned this pull request Sep 16, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants