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

Hoist preference enums to shorten imports #5915

Open
sffc opened this issue Dec 18, 2024 · 1 comment
Open

Hoist preference enums to shorten imports #5915

sffc opened this issue Dec 18, 2024 · 1 comment
Labels
C-locale Component: Locale identifiers, BCP47

Comments

@sffc
Copy link
Member

sffc commented Dec 18, 2024

Currently if you want to use HourCycle you need to do

use icu_locale_core::preferences::extensions::unicode::keywords::HourCycle;

which is a big much.

I'd be fine with icu::locale::preferences::HourCycle, Or if you want to namespace it with unicode, icu::locale::unicode::HourCycle? I think at least "extensions" and "keywords" are unnecessary components of the module import path for most users. Actually I'd be fine also with icu::locale::enums::HourCycle.

@zbraniecki said:

hmm, I think the taxonomy I had in mind was focused on ability to introduce other preferences reflecting:

  • non extensions (for e.g. TimePattern)
  • other extensions (for e.g. transform)

I can see an argument that those are not going to come in any reasonable future and if they do, they will likely either be more esoteric (so no exposition on top level) or won't collide in names.
As a result, I'm ok with shortening the path to it, I'd suggest keeping the sturucture but exposing all extensions::unicode::* on icu::locale::preferences. How does it sound to you?

I don't like "enums" - they're a Rust type, so it feels like "icu::locale::vectors::*".
I'm ok with icu::locale::preferences::HourCycle

@sffc sffc added C-locale Component: Locale identifiers, BCP47 discuss-priority Discuss at the next ICU4X meeting labels Dec 18, 2024
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Dec 18, 2024
@Manishearth
Copy link
Member

  • @sffc these imports are annoying to users
  • @Manishearth they annoy me too, I know that HourCycle is a unicode extension keyword but it's a pain to remember that I need to put it under preferences/extensions/unicode/keywords
  • @zbraniecki I'm aligned with the simplification. I want to point out 2 types of extensions, an AST extension and the strongly typed extension. Like Value vs HourCycle. My main intention was prevention of collision. Value exists in multiple sub-modules.
  • @sffc My argument only applies to the strongly typed extensions. They are enums. They are defined in their own table in UTS 35 and we can put that table anywhere.
  • @zbraniecki Makes sense.
  • @Manishearth We should be careful about collision between Value types. That might need more scrutiny. So maybe we want a holistic design.
  • @sffc I was coding yesterday in Conformance where serde_json::Value was already imported, so I had to qualify our extensions::unicode::Value. But, I think these are niche types. I don't think we need to decide the answer for both the strongly typed and the AST types at the same time.
  • @zbraniecki Not sure if I'm fully aligned on module qualification being a bad thing.

What should the namespace be called?

  1. icu::locale::preferences::HourCycle
  2. icu::locale::unicode::HourCycle
  3. icu::locale::enums::HourCycle
  4. icu::locale::HourCycle

Discussion:

  • @zbraniecki A bit woried about collisions. But, the use case for these are in options bags. So if there were 2 different emoji or transform, it would collide not only here but also elsewhere. So I hope that across all of the i18n industry, we try to keep the preference unique. But, in ECMA-402 we already have cases where the key of the option bag has a different meaning depending on the class it's used on.
  • @zbraniecki Should we call it prefs instead of preferences?
  • @zbraniecki Or can we put it under ::u? It gives me more confidence that we wouldn't collide.
  • @sffc I don't think it's worth harming the user experience now to protect against future name collisions. I think such collisions would be unlikely, and if they occur, we can just give them a different name.
  • @sffc As far as prefs vs preferences, we already had a bikeshed ballot that chose preferences.
  • @sffc We used the enums naming in icu::datetime::fieldsets and I think that is clear naming. It seems like a good place to go.
  • @Manishearth To go over pros and cons: top-level is fine but cluttered. enums seems fine. I don't like unicode because users shouldn't see that in an identifier; seems inside baseball. Everything we do is "unicode". preferences and enums is also fine.
  • @zbraniecki: naming modules after the incedental structures we use is an antipattern imo, so not enums. Half are not enums, they're structs. Prefer preferences::HourCycle.
  • @sffc I'd prefer enums or types or something but preferences is OK
  • @zbraniecki I think preferences is a better user experience.

Conclusion: icu::locale::preferences::HourCycle

LGTM: @Manishearth @zbraniecki (@sffc)

@Manishearth Manishearth removed the discuss-priority Discuss at the next ICU4X meeting label Dec 19, 2024
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

2 participants