Skip to content

Implement Into<Locale> for preference structs.#7622

Open
jedel1043 wants to merge 4 commits intounicode-org:mainfrom
jedel1043:prefs-into-locale
Open

Implement Into<Locale> for preference structs.#7622
jedel1043 wants to merge 4 commits intounicode-org:mainfrom
jedel1043:prefs-into-locale

Conversation

@jedel1043
Copy link
Copy Markdown
Contributor

This will simplify boa-dev/boa#4589 in the future.

@jedel1043 jedel1043 requested a review from a team as a code owner February 10, 2026 07:09
Copy link
Copy Markdown
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks! Would you mind adding some round-trip test cases?

$(
if let Some(value) = other.$key {
if let Some(ue) = <$pref>::unicode_extension_key() {
let val = value.unicode_extension_value().unwrap();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question/Issue: Will every value always have a unicode extension value?

@robertbastian
Copy link
Copy Markdown
Member

I don't think this makes much sense conceptually. A locale that is obtained from say DateTimeFormatterPreferences cannot be used for anything else, as the conversion into the preferences is lossy. Preferences that are not used by DTF won't round-trip.

@sffc
Copy link
Copy Markdown
Member

sffc commented Feb 10, 2026

The use case is

new Intl.DateTimeFormat("en-u-ca-chinese-co-pinyin").resolvedOptions().locale
// 'en-u-ca-chinese'

@robertbastian
Copy link
Copy Markdown
Member

Preferences are not designed to be the resolved options, they are not aware of any fallback that happens. Does resolvedOptions() just need to throw away irrelevant extensions, or also produce the result of data resolution?

It's very counterintuitive for From to be implemented in both directions, but not round-trip. I think this should be into_locale() or something. Another approach that might suffice for Temporal would be to implement Writeable on preferences to get the string representation.

@sffc
Copy link
Copy Markdown
Member

sffc commented Feb 10, 2026

It appears to do filtering of relevant extension keywords, removing unsupported values, and resolving aliases.

new Intl.DateTimeFormat("en-u-ca-ethiopic-amete-alem-co-pinyin-nu-latn1").resolvedOptions().locale
'en-u-ca-ethioaa'

https://tc39.es/ecma402/#sec-resolvelocale

@sffc
Copy link
Copy Markdown
Member

sffc commented Feb 10, 2026

It's very counterintuitive for From to be implemented in both directions, but not round-trip.

Well, preference-to-locale is the lossless direction. The direction we currently have is the lossy direction.

I think this should be into_locale() or something.

I am always happy with concrete functions over From impls

Another approach that might suffice for Temporal would be to implement Writeable on preferences to get the string representation.

We could do both, but I see no good reason to export Writeable without exporting the Locale conversion, since you can re-parse the writeable into a Locale and just be less efficient.

@robertbastian
Copy link
Copy Markdown
Member

Well, preference-to-locale is the lossless direction. The direction we currently have is the lossy direction.

You can't really talk about lossless if you only have one direction. Right now, we have a Locale domain A and a preferences domain B (which is different from A). From<Locale> for Preferences is a function from domain A to domain B (which is not bijective, but that's fine). As soon as you add the B to A function though, not having bijectivity is very unexpected.

We could do both, but I see no good reason to export Writeable without exporting the Locale conversion, since you can re-parse the writeable into a Locale and just be less efficient.

Yes, but using that Locale is also generally wrong, so I like making it more complicated.

@jedel1043
Copy link
Copy Markdown
Contributor Author

@robertbastian Hmm, what about only implementing a conversion into extensions::unicode::Unicode instead? TBH we don't really need the full Locale in the linked PR, we just need the unicode extensions part. Does that implementation make more sense?

@robertbastian
Copy link
Copy Markdown
Member

I would approve fn as_keywords(&self) -> unicode::Keywords.

@sffc
Copy link
Copy Markdown
Member

sffc commented Feb 10, 2026

A potential issue with that signature is whether we should include -u-rg and -u-sd in the keywords. If we don't include them, then we aren't including all the keywords. If we do include them, then we aren't returning all of the region information.

@Manishearth
Copy link
Copy Markdown
Member

If it's lossy perhaps we should have to_locale_lossy() or to_locale_for_resolved_options()?

@robertbastian
Copy link
Copy Markdown
Member

If we do include them, then we aren't returning all of the region information.

can you elaborate what you mean by this? we're returning all unicode keywords that are relevant to the preferences

@sffc
Copy link
Copy Markdown
Member

sffc commented Feb 11, 2026

If we do include them, then we aren't returning all of the region information.

can you elaborate what you mean by this? we're returning all unicode keywords that are relevant to the preferences

The Preferences struct contains LocalePreferences plus zero or more keywords relevant to the formatter. It seems wrong to pull only some of the data out of LocalePreferences without returning all of the data from LocalePreferences.

@robertbastian robertbastian added discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting and removed discuss Discuss at a future ICU4X-SC meeting labels Feb 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discuss-priority Discuss at the next ICU4X meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants