cldr icon indicating copy to clipboard operation
cldr copied to clipboard

CLDR-18465 Remove Before Diocletian era from Coptic calendar

Open sffc opened this issue 7 months ago • 6 comments
trafficstars

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 avatar Apr 17 '25 01:04 sffc

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

sffc avatar Apr 21 '25 23:04 sffc

What are the next steps on this PR?

sffc avatar Apr 24 '25 21:04 sffc

Hi, can I have a review on this PR?

sffc avatar May 01 '25 23:05 sffc

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.)

macchiati avatar May 01 '25 23:05 macchiati

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 avatar May 23 '25 21:05 sffc

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

@macchiati @pedberg-icu @roozbehp @FrankYFTang

sffc avatar Jun 04 '25 08:06 sffc

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

sffc avatar Jul 23 '25 16:07 sffc

is this still targeted for v48?

robertbastian avatar Aug 26 '25 09:08 robertbastian

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

srl295 avatar Aug 26 '25 11:08 srl295

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 avatar Aug 27 '25 00:08 sffc

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?

sffc avatar Aug 27 '25 01:08 sffc

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.

pedberg-icu avatar Aug 28 '25 05:08 pedberg-icu

Test is here: https://github.com/unicode-org/cldr/blob/58e2f6f7889304691066458bd370c74d6df94463/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCheckCLDR.java#L503

Looks like there are inherited values in bs.xml for example: https://github.com/unicode-org/cldr/blob/58e2f6f7889304691066458bd370c74d6df94463/common/main/bs.xml#L2963

@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

AEApple avatar Sep 04 '25 06:09 AEApple

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 avatar Sep 08 '25 05:09 sffc

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

sffc avatar Sep 08 '25 21:09 sffc

@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!

AEApple avatar Sep 08 '25 21:09 AEApple

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

...

sffc avatar Sep 08 '25 22:09 sffc

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.

AEApple avatar Sep 08 '25 23:09 AEApple

Yay, tests are passing!

sffc avatar Sep 09 '25 00:09 sffc

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.

AEApple avatar Sep 09 '25 01:09 AEApple

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

sffc avatar Sep 09 '25 02:09 sffc

No problem with reordering.

On Mon, Sep 8, 2025, 20:33 Annemarie Apple @.***> wrote:

@.**** approved this pull request.

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

— Reply to this email directly, view it on GitHub https://github.com/unicode-org/cldr/pull/4620#pullrequestreview-3198953717, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACJLEMCBROKC63ZAUC73LU33RZDAVAVCNFSM6AAAAAB3JT332CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCOJYHE2TGNZRG4 . You are receiving this because you were mentioned.Message ID: @.***>

macchiati avatar Sep 09 '25 04:09 macchiati

@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.

macchiati avatar Sep 09 '25 17:09 macchiati

Squashed

sffc avatar Sep 09 '25 19:09 sffc