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

Add icu_preferences util #4996

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

zbraniecki
Copy link
Member

No description provided.

@zbraniecki zbraniecki force-pushed the preferences branch 3 times, most recently from 1360e40 to c2640dc Compare June 5, 2024 19:53
@zbraniecki zbraniecki marked this pull request as ready for review June 5, 2024 19:53
@zbraniecki
Copy link
Member Author

This is ready for review.

The deps in components have been cleaned up and documented, the util iteself is not yet well documented as I prefer to first get aligned on the API.

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

Looks good overall

utils/preferences/Cargo.toml Outdated Show resolved Hide resolved
utils/preferences/src/extensions/unicode/errors.rs Outdated Show resolved Hide resolved
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.

Not a full review yet, just spot-checking

@sffc
Copy link
Member

sffc commented Jun 7, 2024

Discussion notes:

  • @sffc - The list of collator keys is not all that we support. And we should be able to support other custom collator keys.
  • @zbraniecki - The list is easy to extend; I would like to make the list more comprehensive when we actually add the collator preferences. As far as adding a custom variant, that's definitely something we could consider. I would like to defer those specific questions to later because they are compatible with the architecture of icu_preferences.
  • @robertbastian We use TinyStr a lot internally. But you want to switch to using Subtag.
  • @zbraniecki - My preference would be to converge around Subtag, because that's an ICU4X type.
  • @robertbastian Subtag is slightly different; it forces lowercase.
  • @sffc And it requires 2 characters.
  • @robertbastian Maybe we could promote them from being doc(hidden) to being experimental.

components/locale_core/src/extensions/unicode/value.rs Outdated Show resolved Hide resolved
Comment on lines +103 to +105
pub fn into_single_subtag(self) -> Option<Subtag> {
self.0.into_single()
}
Copy link
Member

Choose a reason for hiding this comment

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

Discussion: I don't see a good reason to have a moving into_single_subtag function since Subtag is Copy. Seems like as_single_subtag(&self) would be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

The value is that you don't need to copy even - you destruct into a single Subtag. Let me know if that convinces you, I'll replace it with as_single_subtag().copied() otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just make as_single_subtag return Option<Subtag>.

Copy link
Member

Choose a reason for hiding this comment

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

Optional: rename the function to_single_subtag.

Copy link
Member Author

Choose a reason for hiding this comment

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

as per https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

as_ -> Free       ->  borrowed -> borrowed
to_ -> Expensive  ->  borrowed -> borrowed
                      borrowed -> owned (non-Copy types)
                      owned -> owned (Copy types)
into_ -> Variable ->  owned -> owned (non-Copy types)

We have:

  • as_single_subtag(&self) -> Option<&Subtag>
  • into_single_subtag(self) -> Option<Subtag>

I think as_ and into_ fit - to_ could work if source was Copy, but it's not, so passing it has to consume (to_ should take reference).

Copy link
Member

Choose a reason for hiding this comment

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

Well it's arguable that it is working on Copy types because it returns Some if the thing is a single Subtag (Copy). But I don't feel strongly about this and it need not block the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked that. The closest reference in Stdlib I could find it Cow which has:

  • to_mut(&mut self) -> &mut <B as ToOwned>::Owned
  • into_owned(self) -> <B as ToOwned>::Owned (vs. OSstring::to_owned(&mut self) -> T`.

The latter has also the following description:

Creates owned data from borrowed data, usually by cloning.

So my read is that to_ is meant to be expensive, it takes a borrowed value. Our case is cheap, it takes an owned value.

I agree that it's not a clear cut, but I don't take &self and I don't clone. to_X(self) -> T is only for primitive types such as f64::to_bits.

I'll keep into_*.

utils/preferences/src/extensions/unicode/keywords/emoji.rs Outdated Show resolved Hide resolved
@zbraniecki zbraniecki requested a review from sffc June 10, 2024 01:42
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.

My comment last time was a "here and elsewhere" but the elsewhere didn't happen :)

@zbraniecki zbraniecki merged commit f3b0573 into unicode-org:main Jun 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants