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

ICU-22575 Change AvailableFormatsSink to allow locales to inherit ava… #2707

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

richgillam
Copy link
Contributor

@richgillam richgillam commented Nov 18, 2023

…ilableFormats items from the root locale.

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22575
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@richgillam
Copy link
Contributor Author

NOTE: I think this is the same problem we've been getting reports from various Web users about. A few quick comments:

  • This is the fix I made on the Apple side. There may be a better, cleaner way to do this, although I thought about it a little and this seemed right.
  • If there are new test cases I should add that would correspond better to the reports were getting from the browser people, I'll add them. Can somebody suggest some?
  • I have no idea why we were doing !isRoot before; there didn't seem to be any bad side effects from taking it out.
  • I haven't yet vetted the test changes in this PR; I vetted some of them on the Apple side and confirmed that the new results were the right ones, but I need to go back and do that.
  • I haven't yet ported this change to Java. I'll do that before I check in (but feel free to remind me).
  • I just realized to put this change up against the main branch, but that it's also needed against the maint-74 branch. I can cherry-pick over there and post another PR, but I'd like some validation of this approach first.

Anyway, sorry this is a little half-baked, but I ran out of time and wanted this up before the Thanksgiving break (I'm off all next week) to get feedback and reassure people that the problem was being worked on.

@FrankYFTang
Copy link
Contributor

There should be a Java counter part, right? Could we change the Java one in this PR together?

@richgillam
Copy link
Contributor Author

There should be a Java counter part, right? Could we change the Java one in this PR together?

Of course. I just ran out of time before the break to do that, and I kind of wanted some feedback on what I'm doing and whether it's a good idea or there's a better way to do it. If people are behind this (or don't care), I'll try to put up a new commit that extends the fix to the ICU4J side.

@FrankYFTang
Copy link
Contributor

please fix the failing tests

@markusicu markusicu self-assigned this Nov 30, 2023
@richgillam
Copy link
Contributor Author

Okay, I think this is finally ready to be reviewed. I've fixed the build failures, verified that the changed unit test results make sense, added a new unit test that specifically replicates the problem described in the Mozilla bug report, and ported all of those things to Java. @markusicu @FrankYFTang @pedberg-icu , PTAL.

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.

I looked this over, and it all looks good.

One could also write a little tool that generates patterns from skeletons for different languages, where the skeletons are unions of those of the root and child locales, and captures that in a file. Then run it with and without the code change, and spot-check the differences.

But I'm maybe a bit paranoid..., so glad to approve if you don't have the time to do that.

Copy link
Contributor

@pedberg-icu pedberg-icu left a comment

Choose a reason for hiding this comment

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

The code change and the expected data changes in tests all seem good to me. The expected data changes seem to be places where the CLDR data had ↑↑↑ assuming inheritance from root, so making the ICU code align with that seems good.

@richgillam
Copy link
Contributor Author

One could also write a little tool that generates patterns from skeletons for different languages, where the skeletons are unions of those of the root and child locales, and captures that in a file. Then run it with and without the code change, and spot-check the differences.

But I'm maybe a bit paranoid..., so glad to approve if you don't have the time to do that.

It definitely seems worth doing, but I really don't have the time. The affected cases should pretty much all be situations where CLDR changed a literal value to ↑↑↑ expecting to inherit from root. Although as you can see, making that change uncovered a few unit tests that were expecting something different (e.g., deriving the pattern algorithmically from one of its sibling patterns).

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@richgillam
Copy link
Contributor Author

Thank you for your reviews, @pedberg-icu and @macchiati ! I just squashed my commits-- would somebody mind re-approving?

Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this.

@richgillam richgillam merged commit bcae6f2 into unicode-org:main Dec 4, 2023
103 checks passed
@richgillam richgillam deleted the ICU-22575-inherit-root branch December 4, 2023 20:48
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.

5 participants