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

CLDR-17669 add Organization.airbnb #3747

Merged
merged 5 commits into from
May 23, 2024
Merged

Conversation

srl295
Copy link
Member

@srl295 srl295 commented May 23, 2024

  • Locales.txt

CLDR-17669

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

macchiati
macchiati previously approved these changes May 23, 2024
@srl295
Copy link
Member Author

srl295 commented May 23, 2024

with sr_Cyrl:

Caused by: java.lang.IllegalArgumentException: Cannot have default content locale in Locales.txt: Airbnb ; sr_Cyrl ; modern ; T3 Serbian

versus

without sr_Cyrl:

Error: (TestCLDRLocaleCoverage.java:395) Error: : airbnb; locale=sr_Cyrl_ME; level=modern; parent=sr_Cyrl; level=null: expected true

fix TestCLDRLocaleCoverage to not give incorrect advice:
- default content shouldn't be in Locales.txt so should be skipped. TestStandardCodes tests this.
- clarified TestCLDRLocaleCoverage.TestParentCoverage to identify the two separate failure cases, of missing parentage and child-higher-than-parent
@srl295
Copy link
Member Author

srl295 commented May 23, 2024

Test case was wrong, besides being confusing. take a look

+ parent
+ "; level="
+ parentLevel,
parentLevel != null && parentLevel.compareTo(level) >= 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

two-for-one test condition

@@ -393,16 +393,24 @@ public void TestParentCoverage() {
if (parent == null || parent.equals(LocaleNames.ROOT)) {
break;
}
if (!defaultContentLocales.contains(parent) && !parent.equals("en_001")) { // en_001 is generated later from en_GB
Copy link
Member

Choose a reason for hiding this comment

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

The changes in formatting make it hard to see what the real change was. How did the original of this file escape being formatted by spotless?

Copy link
Member Author

Choose a reason for hiding this comment

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

it didn't escape, it was formatted 'correctly'

The main change is just adding a defaultContent check. The rest is just test output changes.

macchiati
macchiati previously approved these changes May 23, 2024
AEApple
AEApple previously approved these changes May 23, 2024
@srl295 srl295 dismissed stale reviews from AEApple and macchiati via 4be856d May 23, 2024 18:59
@srl295 srl295 merged commit 05e9986 into unicode-org:main May 23, 2024
10 checks passed
@srl295 srl295 deleted the cldr-17669/airbnb branch May 23, 2024 19:18
Airbnb ; sw ; modern ; T3 Swahili
Airbnb ; th ; modern ; T3 Thai
Airbnb ; fil ; modern ; T3 Filipino
Airbnb ; tr ; modern ; T2 Turkish
Copy link
Contributor

Choose a reason for hiding this comment

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

@srl295 - Can we reorder fil so it's alphabetical?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, tl -> fil

yes – want a new PR for it? i'm not sure all of the others are alphabetical though. some might be by tiers

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, tl -> fil

yes – want a new PR for it? i'm not sure all of the others are alphabetical though. some might be by tiers

yes - sgtm! In the past it was by tier. I've been meaning to reorganize the others, but it's been low priority. I've just been adding things alphabetically for anything new.

@DavidLRowe
Copy link
Contributor

Might we want to put CLDR first (by tiers), then other orgs alphabetically? I guess the main goal is to be able to find things easily. Definitely would be nice to have locales listed alphabetically.

@AEApple
Copy link
Contributor

AEApple commented May 23, 2024

Might we want to put CLDR first (by tiers), then other orgs alphabetically? I guess the main goal is to be able to find things easily. Definitely would be nice to have locales listed alphabetically.

There's a different ticket for that. It's just not high priority. I can reassign it to you if you'd like to take it on?

@macchiati
Copy link
Member

macchiati commented May 23, 2024 via email

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.

4 participants