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

Custom variants for icu_preferences #5018

Open
zbraniecki opened this issue Jun 6, 2024 · 3 comments
Open

Custom variants for icu_preferences #5018

zbraniecki opened this issue Jun 6, 2024 · 3 comments
Assignees
Labels
C-locale Component: Locale identifiers, BCP47

Comments

@zbraniecki
Copy link
Member

Spinning out of #4996

In the initial PR I'm introducing closed enums for keys such as Collation. The motivator is that we only need key variants for values we have divergent code or data for, and we control influx of new data and code, so we can start with a key that has 2 variants, and then when a new CLDR drops, or we extend our code to handle a new variant, we just add it to that enum.

@sffc pointed out that there are multiple reasons we may want to support key variants that we do not know about, including custom overrides, and "new data old code" scenarios.
This particularly brings up that we should not exclude ability to select data for a key that our code doesn't know about.

I'll use this issue to discuss solution to that.

@zbraniecki zbraniecki self-assigned this Jun 6, 2024
@zbraniecki
Copy link
Member Author

I have a solution that I can re-add on top of the macros I'm introducing that makes each enum have Custom(unicode::Value) variant that catches all unknown values, making the enum look like:

enum Collation {
    Standard,
    Search,
    Phonetic,
    Pinyin,
    Searchjl,
   Custom(unicode::Value)
}

and then -u-co-search ends up as Collation::Search but -u-co-foo ends up as Collation::Custom(value!("foo")).

This is easy to add, but it brings an architectural question - is there a value in having enum at all then?

For keys that have "open ended" values such as Currency, I use struct Currency(TinyAsciiStr<3>) to represent an open ended validated range of currencies.
If we add Custom, then we could turn Collation, Calendar etc into such struct Collation(Subtag) and struct Calendar(Value) and get rid of enum with known variants at all.

I'm torn on whether this is a better design. I see two use cases of icu_preferences keys API:

  • in code as if preferences.collation == Collation::Search
  • in data as let data = payload.get(preferences.collation)

In the former the ergonomics of having "known" variants is great, while in the latter it doesn't matter - we retrieve a key from some collection by key, so we either need to serialize the known variant, or we can use Subtag/TinyStr/Value as key.

@zbraniecki zbraniecki added this to the ICU4X 2.0 ⟨P1⟩ milestone Jun 7, 2024
@sffc sffc added the C-locale Component: Locale identifiers, BCP47 label Jan 7, 2025
@Manishearth
Copy link
Member

@zbraniecki Questions from 2.0 triage:

  • Is this still an issue?
  • Should this still be a 2.0 blocker?
  • What parts of it are a 2.0 blocker?
  • Do you have a timeline for getting it done?

@zbraniecki
Copy link
Member Author

I don't think this should block 2.0 anymore. It seems like a theoretical addition with no customer in sight.
We ended up defaulting to None for unknown values of preferences which means that we are "forward compatible" in the sense that you can pass en-u-co-foo and we will accept it, it just won't do anything, since we don't store it as a preference for Collation and we don't have data for it.

If one day we add data for it, we will also add the preference, and since our preference enums are #[non_exhaustive] this will be forward compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-locale Component: Locale identifiers, BCP47
Projects
None yet
Development

No branches or pull requests

3 participants