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

[iOS][non-icu] HybridGlobalization implement GetSortKey on hybrid mode #95260

Merged
merged 13 commits into from
Dec 6, 2023
Merged

[iOS][non-icu] HybridGlobalization implement GetSortKey on hybrid mode #95260

merged 13 commits into from
Dec 6, 2023

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Nov 27, 2023 •
edited
Loading

Implements #94216

Contributes to #80689
cc @SamMonoRT

@ghost
Copy link

ghost commented Nov 27, 2023

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Implement GetSortKeyNative on hybrid mode.

Contributes to #80689

Author: mkhamoyan
Assignees: -
Labels:

area-System.Globalization, os-ios
Milestone: -

@ghost ghost assigned mkhamoyan Nov 27, 2023
This was referenced Nov 27, 2023
Closed
Closed
@mkhamoyan mkhamoyan mentioned this pull request Nov 28, 2023
Open
3 tasks
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-maccatalyst

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslikesimulator

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan mkhamoyan marked this pull request as ready for review November 28, 2023 14:53
akoeplinger
akoeplinger approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

One question, looks good otherwise

src/native/libs/System.Globalization.Native/pal_collation.m Outdated Show resolved Hide resolved
This was referenced Nov 28, 2023
Open
Open
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Nov 29, 2023
Closed
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@build-analysis build-analysis bot mentioned this pull request Nov 30, 2023
Closed
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Nov 30, 2023
Open
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

mdh1418
mdh1418 reviewed Dec 4, 2023
View reviewed changes
Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

And as discussed offline, I think it would be good to note somewhere that this implementation does not construct SortKeys like ucol_getSortKey does, and might not adhere to the specifications specifications of SortKey such as SortKeys from different collators not being comparable and merging sortkeys.

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs Outdated Show resolved Hide resolved
src/libraries/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.cs Outdated Show resolved Hide resolved
@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslike

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-maccatalyst

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-ioslikesimulator

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor Author

@dotnet-policy-service rerun

@mkhamoyan
Copy link
Contributor Author

Failures are not related.

@mkhamoyan mkhamoyan merged commit b345e2d into main Dec 6, 2023
211 of 213 checks passed
@mkhamoyan mkhamoyan deleted the get_sort_key_hybrid branch December 6, 2023 10:50
This was referenced Dec 15, 2023
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdh1418 mdh1418 mdh1418 left review comments

@akoeplinger akoeplinger akoeplinger approved these changes

@steveisok steveisok Awaiting requested review from steveisok

Assignees

@mkhamoyan mkhamoyan

Labels
area-System.Globalization os-ios Apple iOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkhamoyan @akoeplinger @mdh1418