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.

Sign up for GitHub

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

Jump to bottom

Struct extensions::unicode::Unicode shouldn't implement Ord #4609

Closed
sffc opened this issue Feb 14, 2024 · 5 comments · Fixed by #5617
Closed

Struct extensions::unicode::Unicode shouldn't implement Ord #4609

sffc opened this issue Feb 14, 2024 · 5 comments · Fixed by #5617
Labels
2.0-breaking Changes that are breaking API changes C-locale Component: Locale identifiers, BCP47 S-tiny Size: Less than an hour (trivial fixes)
Milestone
ICU4X 2.0 ⟨P1⟩

Comments

@sffc
Copy link
Member

sffc commented Feb 14, 2024

The tuple ordering of extensions::unicode::Unicode is not equal to its string ordering. I added a total_cmp function to this type in #4608, and in 2.0 we should remove the derived impl.
@sffc sffc added C-locale Component: Locale identifiers, BCP47 2.0-breaking Changes that are breaking API changes labels Feb 14, 2024
@sffc sffc added this to the ICU4X 2.0 milestone Feb 14, 2024
@sffc sffc added the S-tiny Size: Less than an hour (trivial fixes) label Feb 14, 2024
@Manishearth Manishearth moved this to Tiny (can be done near the end) in icu4x 2.0 Feb 23, 2024
@Harsh1s
Copy link
Contributor

Harsh1s commented Feb 25, 2024

Hi, I'd love to work on this issue. Can you assign this to me?

@sffc
Copy link
Member Author

sffc commented Feb 25, 2024

Hi @Harsh1s! Thanks for your interest but this isn't a good first issue because it is a breaking change that can't land until we start preparing the 2.0 release.

@Harsh1s
Copy link
Contributor

Harsh1s commented Feb 26, 2024

Hi @Harsh1s! Thanks for your interest but this isn't a good first issue because it is a breaking change that can't land until we start preparing the 2.0 release.

Oh, no problem then! Is there any other issue I could work on? I'm quite interested in the project, and would love to make any sort of contribution.

@sffc
Copy link
Member Author

sffc commented Feb 26, 2024

@Harsh1s You can pick an issue with the good first issue label. Some possible suggestions:

#2588

#2454

#2445

#1445

@Harsh1s
Copy link
Contributor

Harsh1s commented Feb 26, 2024

Thanks for the help, hope I can make good contribution to the project!

@Manishearth Manishearth mentioned this issue Sep 30, 2024
Merged
@github-project-automation github-project-automation bot moved this from Small breakage (defer to end) to Done in icu4x 2.0 Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
2.0-breaking Changes that are breaking API changes C-locale Component: Locale identifiers, BCP47 S-tiny Size: Less than an hour (trivial fixes)
Projects
icu4x 2.0
Status: Done
Milestone
ICU4X 2.0 ⟨P1⟩
Development

Successfully merging a pull request may close this issue.

Remove Ord impl from extensions::unicode::Unicode Manishearth/icu4x
2 participants
@sffc @Harsh1s