Skip to content

Conversation

zbraniecki
Copy link
Member

This change aligns with how Preferences::extend works and will be used in regional preferences merges in host_info.

@zbraniecki zbraniecki requested a review from nciric as a code owner October 16, 2025 14:20
@CLAassistant
Copy link

CLAassistant commented Oct 16, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

I think in general this is a good API. I would like @sffc at least to also stamp it.

@Manishearth Manishearth requested a review from sffc October 17, 2025 00:54
Copy link
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.

Seems fine but always concerned about any method named "extend" is the conflict with the std trait Extend https://doc.rust-lang.org/std/iter/trait.Extend.html

I think elsewhere we've called it extend_with_X to avoid the name clash

@zbraniecki
Copy link
Member Author

Hmm, I missed it here: https://docs.rs/icu/latest/icu/locale/preferences/struct.LocalePreferences.html#method.extend - which is what I wanted to shape it after.

I looked at potentially using this trait, and:

  1. For Attributes and Keywords I can use it, but I need to introduce IntoIterator for both, and this has to be behind alloc because the IntoIterator for ShortBoxSlice is. It's slightly unfortunate but since my use case (extend) will be behind alloc anyway, I think it's acceptable.
  2. I can't use it for Unicode itself. It's not a collection and doesn't work with iteration.

Now that I reflect on it, I think I introduced the method on Preferences because the Extend trait only work on collections, meaning that each struct is either a collection and may implement the trait or not a collection and cannot.
So Unicode could have a method extend, just like Preferences do, and for Attributes and Keywords we can decide if we want to use the trait + IntoIterator or own method.

@Manishearth @sffc - what are your preferences here?

@zbraniecki zbraniecki added the discuss Discuss at a future ICU4X-SC meeting label Oct 17, 2025
@sffc
Copy link
Member

sffc commented Oct 17, 2025

Yeah if something is not collection-like, and won't ever implement the Extend trait, then it seems harmless to use a function named extend.

LocalePreferences (and Locale in general) are heterogenous collections, as in, you can't really turn them into iterators of something. So maybe extend is fine.

Here's where I used extend_from_litemap in LiteMap:

https://docs.rs/litemap/latest/litemap/struct.LiteMap.html#method.extend_from_litemap

@zbraniecki
Copy link
Member Author

@sffc what's your take on IntoIterator on Attributes/Keywords being behind alloc?

Copy link
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.

Observation: Keywords already implements iter(&self) -> impl Iterator<Item = (&Key, &Value)> so it's already public that we consider it collection-like, so I think it's safest to not use a custom extend function signature. However, I think it's overkill to use the trait. I suggest using the specific function names on Keywords and Attributes. However, since Extensions is heterogeneous, I don't think we need to use the special name there.

/// assert_eq!(kw, "ab-cd-ca-gregory-hc-h12".parse().unwrap());
/// ```
#[cfg(feature = "alloc")]
pub fn extend(&mut self, other: Keywords) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn extend(&mut self, other: Keywords) {
pub fn extend_from_keywords(&mut self, other: Keywords) {

/// assert_eq!(attrs, "foobar-foobaz-fooqux".parse().unwrap());
/// ```
#[cfg(feature = "alloc")]
pub fn extend(&mut self, other: Attributes) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn extend(&mut self, other: Attributes) {
pub fn extend_from_attributes(&mut self, other: Attributes) {

@Manishearth
Copy link
Member

I also think it's overkill to use the trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discuss Discuss at a future ICU4X-SC meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants