Skip to content
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

Upgrade to CLDR 45 #1077

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Upgrade to CLDR 45 #1077

wants to merge 2 commits into from

Conversation

tomasr8
Copy link
Contributor

@tomasr8 tomasr8 commented May 1, 2024

CLDR 45 is out now.
Looks like the 44 patch fixed most of the test issues so there was only one failing test this time around :)

Some related discussion here: #1070

Copy link

codecov bot commented May 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.32%. Comparing base (e0d1018) to head (15daada).
Report is 6 commits behind head on master.

❗ Current head 15daada differs from pull request most recent head b0d788f. Consider uploading reports for the commit b0d788f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
- Coverage   90.99%   90.32%   -0.68%     
==========================================
  Files          26       26              
  Lines        4444     4453       +9     
==========================================
- Hits         4044     4022      -22     
- Misses        400      431      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

LGTM, that was simple enough. 😄

However, the release notes say "In Parent Locales, made substantial changes to the way that parentLocales work, including a new attribute for algorithmic handling of inheritance that avoids needing a long (and fragile) list of language-script codes to skip when falling back to root. That list was retained for migration, but will be withdrawn in the future."

Would you like to see whether that's something that affects us and implement whatever changes are relevant in scripts/import_cldr.py? If you don't have the bandwidth, it's fine, I can also follow up here.

@tomasr8
Copy link
Contributor Author

tomasr8 commented May 7, 2024

Would you like to see whether that's something that affects us and implement whatever changes are relevant in scripts/import_cldr.py? If you don't have the bandwidth, it's fine, I can also follow up here.

I might have some time this week, but feel free to have a look yourself if you want :)

@akx akx added this to the Babel 2.16 milestone May 7, 2024
Locales of the form 'lang_Script' where 'Script' is not the
likely script for 'lang' should have 'root' as their parent
locale. For example, the parent of 'az_Arab' should not be
computed as 'az' by truncating from the end, but should be
'root' instead as 'Arab' is not the likely script for 'az'.

The list of such languages was previously specified using
an explicit 'locales' attribute. It is now handled dynamically
using the new 'localeRules' attribute.
@tomasr8
Copy link
Contributor Author

tomasr8 commented May 9, 2024

I updated the PR to handle the parent locale changes. I just hope I understood it correctly 😅

Comment on lines +156 to +157
if _is_non_likely_script(name):
parent = 'root'
Copy link
Member

Choose a reason for hiding this comment

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

Could we have some sort of test case for this code path? I'm not sure which locales might exhibit this, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already covered by the second test case with az_Arab. Do you want me to add another test for _is_non_likely_script as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants