Replace `DataLocale` by `LanguageIdentifier` on `DataRequest` #4984

robertbastian · 2024-06-01T00:57:51Z

Unicode extension keys are now modeled as DataKeyAttributes.

#3632

sffc

Praise: It looks approximately what I expect to see but it's a big PR so I haven't finished looking at every file

components/calendar/src/any_calendar.rs

sffc

Down through provider/core; next stop provider/datagen

sffc · 2024-06-03T16:14:12Z

components/collator/src/comparison.rs

+        let metadata_payload: DataPayload<crate::provider::CollationMetadataV1Marker> = provider
+            .load(req)
+            .or_else(|_| {
+                provider.load(DataRequest {
+                    langid: req.langid,
+                    ..Default::default()
+                })
+            })?
+            .take_payload()?;


Suggestion, here and elsewhere: perhaps refactor to make more clear why you are making two requests

Suggested change

let metadata_payload: DataPayload<crate::provider::CollationMetadataV1Marker> = provider

.load(req)

.or_else(|_| {

provider.load(DataRequest {

langid: req.langid,

..Default::default()

})

})?

.take_payload()?;

let metadata_payload: DataPayload<crate::provider::CollationMetadataV1Marker> = provider

.load(req_specific)

.or_else(|_| {

provider.load(req_default)

})?

.take_payload()?;

sffc · 2024-06-03T16:18:29Z

components/datetime/tests/datetime.rs

                            Default::default(),
                        )
                        .unwrap();
                        assert_writeable_eq!(
                            dtf.format(&datetime),
                            *expected,
                            "\n\
-                            locale:   `{}`,\n\
+                            locale:   `{:?}`,\n\


Thought/Issue: We probably want to keep a Display impl on Preferences?

I don't want to make that assumption now. It's not a problem to use Debug in a test.

sffc · 2024-06-03T16:21:29Z

components/locale/src/fallback/algorithms.rs

-                // Always retain -u-sd
-                SUBDIVISION_KEY => true,


Thought/Issue: We don't currently use -u-sd but it would be nice to support it in LocaleFallbacker for future compatibility

This is a new requirement, please don't block the PR on this.

I won't block the PR on it but it's not a new requirement; the old fallbacker supported -u-sd so it's a regression in behavior that it doesn't support it now and doesn't have a clear path to supporting it...

We had a design discussion in March to drop unicode keywords from the fallbacker and this was not brought up since then. This will require a lot of rethinking.

sffc · 2024-06-03T16:29:33Z

components/locale/src/fallback/algorithms.rs

-        TestCase {
-            // TODO(#4413): -u-rg is not yet supported; when it is, this test should be updated
-            input: "en-u-rg-gbxxxx",
-            requires_data: false,
-            extension_key: None,
-            fallback_supplement: None,
-            expected_language_chain: &["en"],
-            expected_region_chain: &["en"],
-        },


Issue: We should fix this in 2.0 because it might require an architecture change in LocaleFallabcker. #4413

sffc · 2024-06-03T16:39:32Z

provider/core/src/hello_world.rs

+        ("en-019", "", "Hello from 🌎"),       // AMERICAS
+        ("en-142", "", "Hello from 🌏"),       // ASIA
+        ("en-GB", "", "Hello from 🇬🇧"),        // GREAT BRITAIN
+        ("en-GB", "england", "Hello from 🏴󠁧󠁢󠁥󠁮󠁧󠁿"), // ENGLAND


Issue: england should remain a subdivision, not a key attribute... please at least add a TODO(#4413)

This is as designed

sffc · 2024-06-03T17:00:35Z

Reasons we may want to keep DataLocale and not use LanguageIdentifier:

We can make it Copy by supporting only a single variant
We can support subdivision
We can add other helper functions and are not bound to the constraints of LanguageIdentifier

robertbastian · 2024-06-03T17:06:38Z

Reasons we may want to keep DataLocale and not use LanguageIdentifier:

We had agreed on this and I have implemented this, which was a non-trivial amount of work.

We can make it Copy by supporting only a single variant

What does this gain us?

We can support subdivision

Ok, but there are other ways as well

We can add other helper functions and are not bound to the constraints of LanguageIdentifier

Having logic duplicated between DataLocale and icu_locale really bothered me about the old DataLocale, because that was code duplication, as well as double the API surface, with subtle API differences. I don't see LanguageIdentifier as a constraint, if we need more functionality we should add it there so that other users can benefit from it (like I added is_und in this PR).

sffc · 2024-06-03T17:14:31Z

Reasons we may want to keep DataLocale and not use LanguageIdentifier:

We had agreed on this and I have implemented this, which was a non-trivial amount of work.

We hadn't considered the restriction of being unable to support u-sd as easily unfortunately

We can make it Copy by supporting only a single variant

What does this gain us?

It's a little cheaper 🤷

We can support subdivision

Ok, but there are other ways as well

How? Let's discuss

We can add other helper functions and are not bound to the constraints of LanguageIdentifier

Having logic duplicated between DataLocale and icu_locale really bothered me about the old DataLocale, because that was code duplication, as well as double the API surface, with subtle API differences. I don't see LanguageIdentifier as a constraint, if we need more functionality we should add it there so that other users can benefit from it (like I added is_und in this PR).

I understand this point of view but we made the conscious decision to duplicate the API surface in 1.0 because we established the understanding that the needs of locales in the context of data providers was different than the needs of locales in the context of UTS 35. (I was hopeful that the new DataRequest could maybe allow us to align, but I've never been fully convinced that merging DataLocale back into LanguageIdentifier was worthwhile)

sffc · 2024-06-03T20:44:46Z

^ If we can demonstrate that there's a good way to support -u-sd and -u-rg in this design, I'm onboard

Manishearth · 2024-06-03T20:53:53Z

(currently feel like no strong opinion on whether we should land this. happy to take part in discussions, but no need to wait for my review, quickly going through this I don't see a reason why I would block this)

sffc · 2024-06-06T19:27:02Z

Current thinking of @sffc @zbraniecki:

Subtags could be about preferences/hints and data identity.
-u-sd is either a preference (I want Scottish unit preferences) or a data identity (I want Catalan as spoken in Valencia)
Variants are a grab bag that encode different concepts such as orthography (POLYTON), subdivision (VALENCIA), and operating system (MACOS), and probably other things, too. (full list)
@zbraniecki notes that subdivision suffixes are 1-3 alphanums. This means in principle we could encode them in an ICU4X language identifier following a region, such as en-US-ca, without risking conflict with variants or other subtags. When converting to Locale, we make it en-US-u-sd-usca, and when converting to LanguageIdentifier, we drop the subdivision. ca_ES_VALENCIA would eventually become ca-ES-vc. Would also be nice to drop the variant subtag.
We could consider using this new type in Preferences. Multi-variants would be mapped to their respective proper keywords, with keywords added as necessary.
@sffc notes that variants could be used by prepending "sd", such as en-US-sdusca (guaranteed to be 5-7 alphanums), if we wanted to stick with the traditional LanguageIdentifier representation.
Would like to bring this to the CLDR Design WG for further discussion.

sffc · 2024-06-06T21:57:12Z

Design doc: https://docs.google.com/document/d/1Rrp1IxycTu2CETceUOQRcwVECXsv0pOJIAUMPdYlHYY/edit

zbraniecki · 2024-06-07T01:30:40Z

Subtle adjustment:

-u-sd is either a preference (I want Scottish unit preferences) or a data identity (This is Valencian data)

sffc · 2024-06-11T13:51:06Z

We discussed this at the CLDR design meeting. There wasn't a clear conclusion on what fields should be in data locale, but it was evident that this could change over time. An approach that seems like it would make sense for us would be to restrict to a single subtag on top of LSR which contains an implementation-defined enumeration of data identity. For example, we lump all variants and subdivisions into our own namespace.

robertbastian · 2024-06-20T12:53:48Z

Discussion

#[derive(Copy, ...)]
pub struct DataLocale {
    language: icu_locale_core::subtags::Language,
    script: Option<icu_locale_core::subtags::Script>,
    region: Option<icu_locale_core::subtags::Region>,
    subdivision: Option<icu_locale_core::extensions::unicode::SubdivisionSuffix>,
    single_variant: Option<icu_locale_core::subtags::Variant>,
}

simplify DataLocale API by removing extensions stuff
have getters and setters for the singleton fields, as well as is_und/is_empty/is_default
move to icu_preferences?
fallback still uses DataLocale

robertbastian added 7 commits June 1, 2024 02:56

locale on api

274bb4f

mutators

7dca21c

d

ee47627

dv

1959219

d

c4dab5f

f

d5eeee5

dart

9ae7f1a

robertbastian marked this pull request as ready for review June 1, 2024 01:33

robertbastian requested review from sffc, Manishearth, filmil, aethanyc, makotokato, zbraniecki, nciric, dminor, pdogr, snktd, eggrobin, nordzilla, hsivonen, echeran and a team as code owners June 1, 2024 01:33

robertbastian removed request for aethanyc, filmil, dminor, hsivonen, echeran, makotokato and eggrobin June 1, 2024 01:34

robertbastian added 5 commits June 1, 2024 11:18

fix collator

e4f1f3f

silent

365f543

extension key

0ba6422

revert

12f403d

doc

cc5f27f

robertbastian changed the title ~~Replace DataLocale by LanguageIdentifier on DataRequest, Locale on API~~ Replace DataLocale by LanguageIdentifier on DataRequest, ~Locale on API~ Jun 1, 2024

robertbastian changed the title ~~Replace DataLocale by LanguageIdentifier on DataRequest, ~Locale on API~~~ Replace DataLocale by LanguageIdentifier on DataRequest Jun 1, 2024

robertbastian added 2 commits June 1, 2024 14:05

cmpact

e6769a2

fmt

13dedca

sffc reviewed Jun 3, 2024

View reviewed changes

components/calendar/src/any_calendar.rs Outdated Show resolved Hide resolved

wip

72d0c07

robertbastian requested a review from sffc June 3, 2024 15:30

sffc reviewed Jun 3, 2024

View reviewed changes

Merge branch 'main' into u-

c20b20e

robertbastian requested a review from sffc June 3, 2024 17:06

sffc added the discuss-priority Discuss at the next ICU4X meeting label Jun 4, 2024

sffc mentioned this pull request Jun 12, 2024

Remove -x- encoding for marker attributes #5038

Merged

robertbastian closed this Jun 20, 2024

robertbastian mentioned this pull request Jul 1, 2024

Use TinyAsciiStr instead of &'static str for baked data #5159

Closed

robertbastian deleted the u- branch August 14, 2024 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `DataLocale` by `LanguageIdentifier` on `DataRequest` #4984

Replace `DataLocale` by `LanguageIdentifier` on `DataRequest` #4984

robertbastian commented Jun 1, 2024 •

edited

Loading

sffc left a comment

sffc left a comment

sffc Jun 3, 2024

sffc Jun 3, 2024

robertbastian Jun 3, 2024

sffc Jun 3, 2024

robertbastian Jun 3, 2024

sffc Jun 3, 2024

robertbastian Jun 3, 2024

sffc Jun 3, 2024

sffc Jun 3, 2024

robertbastian Jun 3, 2024

sffc commented Jun 3, 2024

robertbastian commented Jun 3, 2024

sffc commented Jun 3, 2024 •

edited

Loading

sffc commented Jun 3, 2024

Manishearth commented Jun 3, 2024

sffc commented Jun 6, 2024

sffc commented Jun 6, 2024

zbraniecki commented Jun 7, 2024

sffc commented Jun 11, 2024

robertbastian commented Jun 20, 2024

Replace DataLocale by LanguageIdentifier on DataRequest #4984

Replace DataLocale by LanguageIdentifier on DataRequest #4984

Conversation

robertbastian commented Jun 1, 2024 • edited Loading

sffc left a comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Jun 3, 2024

robertbastian commented Jun 3, 2024

sffc commented Jun 3, 2024 • edited Loading

sffc commented Jun 3, 2024

Manishearth commented Jun 3, 2024

sffc commented Jun 6, 2024

sffc commented Jun 6, 2024

zbraniecki commented Jun 7, 2024

sffc commented Jun 11, 2024

robertbastian commented Jun 20, 2024

Replace `DataLocale` by `LanguageIdentifier` on `DataRequest` #4984

Replace `DataLocale` by `LanguageIdentifier` on `DataRequest` #4984

robertbastian commented Jun 1, 2024 •

edited

Loading

sffc commented Jun 3, 2024 •

edited

Loading