-
Notifications
You must be signed in to change notification settings - Fork 174
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 a collator implementation under experimental/ #1706
Conversation
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments on datagen/provider stuff
Also, I cannot comment this in the file because it's too big:
nit: move experimental/collator/src/CollationTest_*.txt
to a testdata
directory
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to stick to Conventional Comments, but I forgot that all of these non-optional comments need to be accompanied with the proper Github review flags being set.
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had written some comments but forgot to send them, they might be outdated.
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Co-authored-by: Robert Bastian <robertbastian@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for everything outside experimental
; I haven't looked at the actual algorithms in-depth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wait for Elango's review before submitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of minor things, but feel free to merge this in now, and create a followup PR later.
@hsivonen Feel free to hit the merge button, and start opening smaller PRs to resolve remaining open comments. Please make sure all the open comments are associated with an open issue. Great work! |
Thanks. Merged. I will make sure issues are filed for the comments. |
I'd like to land the current collator work in progress under
experimental/
, so that it would participate in provider refactorings without having to catch up on a branch.What works
&str
(guaranteed-valid UTF-8),&[u8] (potentially-invalid UTF-8), and
&[u16]` (potentially-invalid UTF-16) comparison.CollationTest_CLDR_NON_IGNORABLE.txt
andCollationTest_CLDR_SHIFTED.txt
What doesn't work
en
asund
.)Post-landing help would be particularly helpful on
searchlj
. (Per meeting: Via-u-co-
in theLocale
.)zh-HK
,zh-MO
,zh-TW
,zh-Hant
,zh-Hant-HK
,zh-Hant-MO
,zh-Hant-TW
tozh
with variantstroke
.(I've edited this text to reflect the code updates.)