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-16403 Add v44 PersonName features #3304

Merged

Conversation

macchiati
Copy link
Member

@macchiati macchiati commented Oct 3, 2023

CLDR-16403

There is a lot of overlap between this ticket and https://unicode-org.atlassian.net/browse/CLDR-17124. I went ahead and included the extra text for nativeSpaceReplacement (replacing some old text), so I think we can mark https://unicode-org.atlassian.net/browse/CLDR-17124 as a duplicate.

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

pedberg-icu
pedberg-icu previously approved these changes Oct 3, 2023
@macchiati macchiati requested a review from pedberg-icu October 3, 2023 15:18
@macchiati
Copy link
Member Author

Ping?

@pedberg-icu
Copy link
Contributor

Ping?

Looking at it now, was backed up behind some other also-urgent things

@@ -135,6 +135,13 @@ The following features are currently out of scope for Person Names formating:

A draft API for formatting personal names is included in ICU4J 73. (“Draft” means that the full functionality is present, but the API might be refined before it is stabilized.) The implementation can be found at [PersonNameFormatter.java](https://github.com/unicode-org/icu/blob/main/icu4j/main/classes/core/src/com/ibm/icu/text/PersonNameFormatter.java) and [SimplePersonName.java](https://github.com/unicode-org/icu/blob/main/icu4j/main/classes/core/src/com/ibm/icu/text/SimplePersonName.java).
Copy link
Contributor

Choose a reason for hiding this comment

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

I will make a separate PR to update this line.

@pedberg-icu
Copy link
Contributor

I am going to go ahead and merge this. Any follow-on fixes can go into a separate PR.

@pedberg-icu pedberg-icu merged commit 2dd770a into unicode-org:main Oct 3, 2023
@macchiati macchiati deleted the CLDR-16403-Add-v44-Personname-features branch October 4, 2023 09:26
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Belated review. Generally looks great, but there were a couple things in here that surprised me-- I've called those out below.


1. forceGivenFirst — no matter what the values are in nameOrderLocales or in the NameObject, display the name as givenFirst.
2. forceSurnameFirst — no matter what the values are in nameOrderLocales or in the NameObject, display the name as surnameFirst.
3. forceNativeOrdering — no matter what the values are in nameOrderLocales or in the NameObject, display the name with the same ordering as the native locale.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters, but I didn't implement this in ICU. I think that was partly because what we had at the time wasn't clear on just what this was supposed to do, but also because what I thought it was supposed to do was going to require a lot of extra work in the code that wouldn't normally be worth it. Is this really worth doing? If so, you might want to file an ICU ticket.

1. forceGivenFirst — no matter what the values are in nameOrderLocales or in the NameObject, display the name as givenFirst.
2. forceSurnameFirst — no matter what the values are in nameOrderLocales or in the NameObject, display the name as surnameFirst.
3. forceNativeOrdering — no matter what the values are in nameOrderLocales or in the NameObject, display the name with the same ordering as the native locale.
4. surnameFirstAllCaps — display the surname and surname2 fields in all caps **if** not using native order. Thus where the foreign name ordering is surnameFirst, the name {given=Shinzo, surname=Abe} would display as “ABE Shinzo”.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I implemented this this way-- I think my ICU code always capitalizes the surname, regardless of field order. This is the first I've heard that we wanted to restrict that behavior to just names that are displayed in the opposite of their normal order. Again, (without looking at the code right now) this seems like a significant amount of extra work at run time to get and check the order-- is it really worth it? If so, please file an ICU ticket.

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