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

Baked data for icu_properties #3565

Merged
merged 5 commits into from
Jun 26, 2023
Merged

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Jun 22, 2023

#2945

Fixes #3565

This is not release-polished, but is currently blocking segmenter, collator, and normalizer baked data, so let's bikeshed names and docs later. Potentially relevant: #2856 (comment)

@robertbastian robertbastian requested a review from a team as a code owner June 22, 2023 11:59
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.

(would like to block on me reviewing this one since there are a couple api choices)

@robertbastian
Copy link
Member Author

(squash)

Manishearth
Manishearth previously approved these changes Jun 22, 2023
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.

Since you have a bunch of PRs depending on this one I am fine with this landing now but please address the comments in a followup. (The naming one we can discuss, I don't have strong opinions but I wanted to flag it)

components/properties/src/sets.rs Outdated Show resolved Hide resolved
components/properties/src/sets.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.

There's a lot of stuff in this PR; not done reading it

components/properties/src/bidi_data.rs Outdated Show resolved Hide resolved
components/properties/src/exemplar_chars.rs Outdated Show resolved Hide resolved
@robertbastian
Copy link
Member Author

I'd really prefer to do the name bikeshedding later as this blocks other PRs.

@Manishearth
Copy link
Member

(I am ok with this landing as-is with all the comments considered for followup)

sffc
sffc previously approved these changes Jun 23, 2023
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.

OK to fix in follow ups

components/properties/src/bidi_data.rs Show resolved Hide resolved
components/properties/src/exemplar_chars.rs Show resolved Hide resolved
components/properties/src/maps.rs Outdated Show resolved Hide resolved
components/properties/src/props.rs Outdated Show resolved Hide resolved
Manishearth
Manishearth previously approved these changes Jun 26, 2023
Comment on lines +26 to +27
//! assert!(sets::emoji().contains('🎃')); // U+1F383 JACK-O-LANTERN
//! assert!(!sets::emoji().contains('木')); // U+6728
Copy link
Member

Choose a reason for hiding this comment

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

Nit, here and elsewhere (optional): I have a slight preference for keeping the intermediate object stored in its own variable since it is closer to what the code looks like if you use the explicit data provider version of it

@sffc sffc merged commit c394c1c into unicode-org:main Jun 26, 2023
@sffc
Copy link
Member

sffc commented Jun 26, 2023

Force-merging because FFI CI is not working right now (let's see if it works on main)

@robertbastian robertbastian deleted the properties branch June 26, 2023 20:38
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