-
Notifications
You must be signed in to change notification settings - Fork 387
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-17563 fix unclear test message about attributeValueValidity.xml #3647
base: main
Are you sure you want to change the base?
Conversation
- fix an internal function for clarity - change a constant to a property - [X] be less cryptic - drop debug code
97c0b19
to
7ba8ec9
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
trying again without using an unrelated ticket ID |
Not clear on the different ticket numbers (CLDR-17563 in title, CLDR-16818 in description). From the little I know about Java, the error message seems much more informative! |
Fixed |
@@ -1280,7 +1278,7 @@ public void testLSR() { | |||
} | |||
} | |||
if (!assertEquals( | |||
"Language subtags that are in a CLDR locale's ID are in root (" | |||
"$language in attributeValueValidity.xml needs to include these languages: (" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misleading in a different way. The error just indicates that there is a mismatch.
"$language in attributeValueValidity.xml needs to include these languages: (" | |
"There is a mismatch: each language in the following list either needs to be removed from root.xml, OR added to $language in attributeValueValidity.xml: (" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "removed from root.xml" mean? In the reproducer, "lld" wasn't in root.xml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macchiati any clarification on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. Why was lld failing; was it for not being in validity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. lld had not been added to validity element in attributeValueValidity.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3666 changes this wording to: "Language subtags in a locale's ID must be in one of the attributeValueValidity.xml $language* sets, typically $languageNonTcLtBasic ("
which is even clearer. I'd also recommend dropping the message change from this PR.
I won't have time to look at this today.
…On Fri, Apr 19, 2024 at 12:22 PM Steven R. Loomis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCoverageLevel.java
<#3647 (comment)>:
> @@ -1280,7 +1278,7 @@ public void testLSR() {
}
}
if (!assertEquals(
- "Language subtags that are in a CLDR locale's ID are in root ("
+ "$language in attributeValueValidity.xml needs to include these languages: ("
What does "removed from root.xml" mean? In the reproducer, "lld" wasn't in
root.xml.
—
Reply to this email directly, view it on GitHub
<#3647 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJLEMDXM6MPHT2SH2VRU53Y6FVG7AVCNFSM6AAAAABGPNV3Y2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJSGIYDONRTHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
The other data pr is unblocked. This is just to clarify. |
As a part of #3666, I fixed the messages and added a lot of comments about what is going on in that section. Should be much easier for people to see what is going on. If that is approved, then I think you could drop the rewording part of this PR (but keep the fix to SHOW_LSR_DATA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see recommendation to drop message change (since another PR will address that).
CLDR-17563
ALLOW_MANY_COMMITS=true
Example run (fake of course):