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-17088 Flesh out en.xml #4110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

btangmu
Copy link
Member

@btangmu btangmu commented Oct 5, 2024

-New TestEnInheritance.java checks modern coverage inherited values in en that are marked with up arrows or are blank

-Make TestPaths.extraPathAllowsNullValue public and static for use by TestEnInheritance

-New CLDRModify option -fE corrects problems that would fail TestEnInheritance

-Revise common/main/en.xml per running CLDRModify -fE

CLDR-17088

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-New TestEnInheritance.java checks modern coverage inherited values in en that are marked with up arrows or are blank

-Make TestPaths.extraPathAllowsNullValue public and static for use by TestEnInheritance

-New CLDRModify option -fE corrects problems that would fail TestEnInheritance

-Revise common/main/en.xml per running CLDRModify -fE
@btangmu btangmu self-assigned this Oct 5, 2024
@btangmu btangmu marked this pull request as ready for review October 7, 2024 12:54
@btangmu
Copy link
Member Author

btangmu commented Oct 7, 2024

Please note: I just made this ready for review. However it's targeted for v47 not v46, and I'm not sure where we stand with the main branch and v46 vs v47. This PR is not urgent.

@srl295
Copy link
Member

srl295 commented Oct 8, 2024

Please note: I just made this ready for review. However it's targeted for v47 not v46, and I'm not sure where we stand with the main branch and v46 vs v47. This PR is not urgent.

open for merges.

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.

Do you really mean isBlank in the sense of Java, that is, consists of 0 or more whitespace characters?

@btangmu
Copy link
Member Author

btangmu commented Oct 9, 2024

Do you really mean isBlank in the sense of Java, that is, consists of 0 or more whitespace characters?

The first comment in the ticket includes "Add a test to check modern coverage inherited values in en that are marked with up arrows or are blank". I don't know whether the intended meaning of "blank" matches the Java method. I reasoned that a broad interpretation would bring up any questionable values, and the method could be revised accordingly as needed.

My first draft PR (#3444) implemented the test with isBlank, and you commented "There are a few values that can be blank, so they would need to be excluded." In response, I added exceptions for DisplayAndInputProcessor.FSR_START_PATH/NSR_START_PATH. The result is that no "blank" values are currently found by the test.

(BTW this PR does change ↑↑↑ to space for FSR_START_PATH.)

We could remove the code changes in this PR that involve isBlank, and I think it would make no difference to the data changes or the test results for this PR. After future changes to en.xml, the potential consequences for future runs of the test and the modify -fE tool aren't clear to me.

Please let me know whether further changes are appropriate.

@macchiati
Copy link
Member

I see what the problem is. Comment https://unicode-org.atlassian.net/browse/CLDR-17088?focusedCommentId=173120 says:

Add a test to check modern coverage inherited values in en that are marked with up arrows or are blank; all values should be explicit in en

It should be:

Add a test to check for modern coverage paths in en.xml that are inherited (the value is either up arrows or null); all such paths need to have explicit values in en.xml. This includes all directories with inheritance, so main/, annotations/, and annotationsDerived.

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.

3 participants