Add missing keys to `CollatorPreferences` #5950

jedel1043 · 2025-01-01T19:20:47Z

Adds support for kn and kf on CollatorPreferences.

hsivonen · 2025-01-02T07:28:18Z

Looks OK to me, but I let @zbraniecki review the macro usage.

zbraniecki · 2025-01-02T11:57:19Z

Thank you for the PR! The only thing I'd like to ponder over is that you're introducing an example where our sepration of Preferences and Options gets blurry - the developer sets an option, and user may set another. We have to decide who takes precedence.

jedel1043 · 2025-01-02T15:27:47Z

@zbraniecki FWIW, on ECMA402 explicit configs take priority over unicode keys, so I expressed that here. If we want to add more flexibility for it, I think we can add an option to explicitly prioritise either one of them.

zbraniecki · 2025-01-02T16:22:05Z

components/collator/src/options.rs

+            CollationCaseFirst::Upper => CaseFirst::UpperFirst,
+            CollationCaseFirst::Lower => CaseFirst::LowerFirst,
+            CollationCaseFirst::False => CaseFirst::Off,
+            _ => unreachable!("`CaseFirst` and `CollationCaseFirst` must always be in sync."),


q: Should we panic here or debug_assert and in release select [default]?

zbraniecki · 2025-01-02T16:24:23Z

components/collator/src/options.rs

+            CaseFirst::Off => CollationCaseFirst::False,
+            CaseFirst::LowerFirst => CollationCaseFirst::Lower,
+            CaseFirst::UpperFirst => CollationCaseFirst::Upper,
+        }


Why you don't need a catch case despite CaseFirst being non-exhaustive?

Should we switch Options to use CollactionCaseFirst from Preferences then?

#[non_exhaustive] only applies for external crates that use the enum.

I preserved the old enum because I wanted to avoid unnecessary breaking changes, but we can switch to that if preferred.

We have a shot at doing it in 2.0, I think i'd prefer that. @sffc @Manishearth @robertbastian - you ok with that?

I have a slight preference for removing the settings from options and moving them to preferences in 2.0

Just to be clear – we would move the relevant options to CollatorPreferences, using the structs on icu_locale_core, and the rest of the options would still be in CollatorOptions, is that right?

jedel1043 · 2025-01-06T06:14:25Z

Ok, switched it to use preference keyword structs and removed the old config structs. Had to slightly adjust the documentation to mention CollatorPreferences, but it should be feature-complete.

zbraniecki · 2025-01-06T08:22:39Z

Oh, interesting. Am I correct to read your PR as - you removed the two preferences from Options, and allow for setting it by the developer by overriding Preferences?

In other words if the developer wants to set one of those two, they interact with Preferences bag, not Options bag anymore?

jedel1043 · 2025-01-06T14:34:50Z

@zbraniecki Yes. That way, the problem we had about priority is solved in an arguably cleaner way, since users can either extend the implicit locale preferences with explicit preferences to give priority to the latter, or do the opposite to give priority to the former.

zbraniecki · 2025-01-06T21:05:18Z

Yes. That way, the problem we had about priority is solved in an arguably cleaner way, since users can either extend the implicit locale preferences with explicit preferences to give priority to the latter, or do the opposite to give priority to the former.

Ok, so you think this is even a cleaner approach! I value your perspective as an implementer. I think I have a hard time deciding on the tradeoffs around it.
My main concern would be that Preferences are intended to be out of control for developers - they accept it (from Locale and from Regional Prefs of their system) and pass it on. The Options on the other hand are meant to be set by the developer. I thought this is a nice clean design but of course reality check is that there are cases where Prefs and Options duplicate and you brought up this here.

With your change we're telling developers to touch Preferences manually. The value of it is that, as you rightly pointed out, it is clean and allows developers to control which direction they want to merge - options->prefs, or prefs->options. ECMA-402 specifies one, but that doesn't mean everyone has to repeat it.

The downsides are:

we make the distinction between Prefs and Options murky. "Prefs are for customers, but if you want to set X, modify them, as you do with Options"
we're now explicitly requiring implementers to think of this nuance and risk getting it wrong or not at all.

I think I'm fine with that as the benefit outweights the cost for me but wanted to have this explicit paper trail of this decision to revisit in the future.

zbraniecki

lgtm! thank you!

sffc

Need to discuss the re-export in #5915 but otherwise this looks great; thanks!

sffc · 2025-01-06T17:37:42Z

components/collator/src/lib.rs

+pub use icu_locale_core::preferences::extensions::unicode::keywords::CollationCaseFirst;
+pub use icu_locale_core::preferences::extensions::unicode::keywords::CollationNumericOrdering;
+pub use icu_locale_core::preferences::extensions::unicode::keywords::CollationType;


Nit: don't pub use; clients should import these from icu::locale.

Unless we want to re-export them? I'm not opposed to that, but we don't do it elsewhere. #5915

Good catch, I think I also prefer clients importing from icu::locale.

jedel1043 · 2025-01-14T23:07:14Z

Ok, made some more changes which include renaming the export from CollationCaseFirst to CaseFirst on the root of icu_collator. This aligns with @sffc's comment, and I think it looks a lot cleaner.

sffc · 2025-01-14T23:48:31Z

components/collator/src/lib.rs

@@ -285,12 +322,13 @@ extern crate alloc;
 pub use comparison::Collator;
 pub use comparison::CollatorBorrowed;
 pub use comparison::CollatorPreferences;
+pub use icu_locale_core::preferences::extensions::unicode::keywords::CollationCaseFirst as CaseFirst;
+pub use icu_locale_core::preferences::extensions::unicode::keywords::CollationNumericOrdering as NumericOrdering;
+pub use icu_locale_core::preferences::extensions::unicode::keywords::CollationType;
 pub use options::AlternateHandling;


Observation: in icu_datetime, options is a public module/namespace. But icu_collator re-exports the things from the top level.

zbraniecki · 2025-01-15T01:39:12Z

Thank you for your work on this @jedel1043 !

jedel1043 requested review from zbraniecki, nciric, hsivonen and echeran as code owners January 1, 2025 19:20

jedel1043 force-pushed the collator-missing-prefs branch from ffa47b4 to 2849caa Compare January 1, 2025 19:26

zbraniecki reviewed Jan 2, 2025

View reviewed changes

jedel1043 requested a review from zbraniecki January 6, 2025 06:19

zbraniecki previously approved these changes Jan 6, 2025

View reviewed changes

zbraniecki requested review from sffc and Manishearth January 6, 2025 21:06

sffc mentioned this pull request Jan 13, 2025

Hoist preference enums to shorten imports #5915

Open

sffc previously approved these changes Jan 13, 2025

View reviewed changes

Add missing keys to CollatorPreferences

aaf1bd8

jedel1043 dismissed stale reviews from sffc and zbraniecki via aaf1bd8 January 14, 2025 22:59

jedel1043 force-pushed the collator-missing-prefs branch from 0f26ab8 to aaf1bd8 Compare January 14, 2025 22:59

cargo make diplomat-coverage

09c63f0

jedel1043 requested review from sffc and zbraniecki January 14, 2025 23:07

sffc added the discuss-priority Discuss at the next ICU4X meeting label Jan 14, 2025

sffc approved these changes Jan 14, 2025

View reviewed changes

zbraniecki approved these changes Jan 15, 2025

View reviewed changes

zbraniecki removed request for hsivonen, echeran, Manishearth and nciric January 15, 2025 01:31

zbraniecki merged commit fc8240b into unicode-org:main Jan 15, 2025
28 checks passed

jedel1043 deleted the collator-missing-prefs branch January 15, 2025 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing keys to `CollatorPreferences` #5950

Add missing keys to `CollatorPreferences` #5950

jedel1043 commented Jan 1, 2025

hsivonen commented Jan 2, 2025

zbraniecki commented Jan 2, 2025

jedel1043 commented Jan 2, 2025 •

edited

Loading

zbraniecki Jan 2, 2025

zbraniecki Jan 2, 2025

jedel1043 Jan 2, 2025

zbraniecki Jan 2, 2025

sffc Jan 4, 2025

Manishearth Jan 5, 2025

jedel1043 Jan 5, 2025

zbraniecki Jan 5, 2025

jedel1043 commented Jan 6, 2025

zbraniecki commented Jan 6, 2025

jedel1043 commented Jan 6, 2025 •

edited

Loading

zbraniecki commented Jan 6, 2025

zbraniecki left a comment

sffc left a comment

sffc Jan 6, 2025

zbraniecki Jan 14, 2025

jedel1043 commented Jan 14, 2025

sffc Jan 14, 2025

sffc Jan 14, 2025

zbraniecki commented Jan 15, 2025

Add missing keys to CollatorPreferences #5950

Add missing keys to CollatorPreferences #5950

Conversation

jedel1043 commented Jan 1, 2025

hsivonen commented Jan 2, 2025

zbraniecki commented Jan 2, 2025

jedel1043 commented Jan 2, 2025 • edited Loading

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

jedel1043 commented Jan 6, 2025

zbraniecki commented Jan 6, 2025

jedel1043 commented Jan 6, 2025 • edited Loading

zbraniecki commented Jan 6, 2025

zbraniecki 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

jedel1043 commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki commented Jan 15, 2025

Add missing keys to `CollatorPreferences` #5950

Add missing keys to `CollatorPreferences` #5950

jedel1043 commented Jan 2, 2025 •

edited

Loading

jedel1043 commented Jan 6, 2025 •

edited

Loading