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

More foo32 consistency #5619

Merged
merged 2 commits into from
Oct 2, 2024
Merged

More foo32 consistency #5619

merged 2 commits into from
Oct 2, 2024

Conversation

robertbastian
Copy link
Member

Manishearth
Manishearth previously approved these changes Sep 30, 2024
/// assert!(!alphabetic.contains32(0x0A69)); // U+0A69 GURMUKHI DIGIT THREE
/// assert!(alphabetic.contains32(0x00C4)); // U+00C4 LATIN CAPITAL LETTER A WITH DIAERESIS
/// ```
/// See [`Self::contains`].
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion, here and elsewhere: Previously the docs said "as a UTF32 code unit" but now it doesn't say that any more.

Suggested change
/// See [`Self::contains`].
/// Returns whether the set contains a code point represented as a UTF-32 code unit.
///
/// For an example, see [`Self::contains`].

Copy link
Member Author

Choose a reason for hiding this comment

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

A u32 is not a UTF-32 code unit though, this was incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

Huh?

It's important to say that the u32 is a UTF-32 code unit. Otherwise, I don't know what it is: it could be a zero-padded ASCII, a Latin1 byte, a TinyStr, …

Copy link
Member

Choose a reason for hiding this comment

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

If you're bothered by "not all u32s are code points", you could stick our agreed-upon term "potential" in there somewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

Please just approve this PR as this is blocking another PR. I can improve docs for all 32 methods in a follow-up.

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.

^

@robertbastian robertbastian merged commit d983fc9 into unicode-org:main Oct 2, 2024
28 checks passed
@robertbastian robertbastian deleted the u32 branch October 2, 2024 15:58
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