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-17680 fix spurious CheckConsistentCasing and CheckCoverage #3767

Merged
merged 2 commits into from
May 31, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented May 30, 2024

CLDR-17680

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCoverage.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

- CheckConsistentCasing was giving a warning for scripts that don't need casing
- CheckCoverage was giving an warning when plurals were == root, rather than
a warning when plurals were missing
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCoverage.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

It is hard to tell whether this is removing the errors from the main page; it doesn't appear that it does.

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

Based on your email, this doesn't remove the messages from the main pages.

Comment on lines -72 to -79
if ((types == null || types.isEmpty()) && !SpecialLocales.isScratchLocale(locale)) {
possibleErrors.add(
new CheckStatus()
.setCause(this)
.setMainType(CheckStatus.warningType)
.setSubtype(Subtype.incorrectCasing)
.setMessage("Could not load casing info for {0}", locale));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, this warning would fire for Arabic, etc. because the script is not cased, so control would reach line 70. Then types is checked again on this line and the warning fires.

The change moves the warning to line 67, so it fires only if casing is expected but not present.

Comment on lines +134 to +136
PluralInfo pluralInfo =
supplementalData.getPlurals(PluralType.cardinal, localeID, false);
if (pluralInfo == null && !SpecialLocales.isScratchLocale(localeID)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, the warning would always fire for Japanese, etc, because the plural rules matched root. This changes to only fire if plural rules were not found.

@srl295
Copy link
Member Author

srl295 commented May 30, 2024

Based on your email, this doesn't remove the messages from the main pages.

It now does

@srl295
Copy link
Member Author

srl295 commented May 30, 2024

It is hard to tell whether this is removing the errors from the main page; it doesn't appear that it does.

I thought issue was just where it was incorrect. It now should remove from all

@srl295 srl295 merged commit c1dc8c7 into unicode-org:main May 31, 2024
11 checks passed
@srl295 srl295 deleted the cldr-17680/casing-skip branch May 31, 2024 00:06
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.

2 participants