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

ICU-22365 C API for ULocaleBuilder #2520

Merged
merged 1 commit into from
Aug 3, 2023
Merged

ICU-22365 C API for ULocaleBuilder #2520

merged 1 commit into from
Aug 3, 2023

Conversation

FrankYFTang
Copy link
Contributor

Still need to add tests

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22365
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang FrankYFTang changed the title ICU-22365 C API for LocaleBuilder ICU-22365 C API for ULocaleBuilder Jun 29, 2023
@FrankYFTang FrankYFTang added the api Adds or changes public API. label Jul 6, 2023
@markusicu markusicu self-requested a review July 6, 2023 16:49
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam
richgillam previously approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Looks good! A few nits and questions, but I'll go ahead and approve.

icu4c/source/common/ulocbuilder.cpp Outdated Show resolved Hide resolved
icu4c/source/common/ulocbuilder.cpp Outdated Show resolved Hide resolved
icu4c/source/common/unicode/ulocbuilder.h Outdated Show resolved Hide resolved
icu4c/source/common/unicode/ulocbuilder.h Outdated Show resolved Hide resolved
icu4c/source/common/unicode/ulocbuilder.h Show resolved Hide resolved
icu4c/source/test/cintltst/ulocbuildertst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocbuildertst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocbuildertst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocbuildertst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocbuildertst.c Outdated Show resolved Hide resolved
@FrankYFTang FrankYFTang added the incomplete Needs work; do not approve/merge as is. label Jul 21, 2023
richgillam
richgillam reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Started to review this again, but it looks like some of the stuff you said wasn't done yet is still in there, so I guess it's not actually ready for me to review again?

icu4c/source/common/ulocbuilder.cpp Outdated Show resolved Hide resolved
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/ulocbuilder.cpp is different
  • icu4c/source/common/unicode/ulocbuilder.h is different
  • icu4c/source/test/cintltst/ulocbuildertst.c is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/cintltst/ulocbuildertst.c is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang removed incomplete Needs work; do not approve/merge as is. waiting-on-author labels Aug 1, 2023
@FrankYFTang
Copy link
Contributor Author

Sorry, now it is ready to review.

richgillam
richgillam previously approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Looks great.

icu4c/source/common/ulocbuilder.cpp Show resolved Hide resolved
FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Aug 1, 2023
@FrankYFTang
ICU-22365 C API for ULocaleBuilder
b923d00 
See unicode-org#2520
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

I corrected the comment per @garywade comments on the API proposal. The additional build method will be added in a separate PR, after the landing of this and the C API for ULocale proposal PR.

@FrankYFTang
Copy link
Contributor Author

@richgillam @garywade - need your final review again. Thanks

richgillam
richgillam approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Rubber-stamp approval. I can't see what you changed since the last time I looked at this (maybe wait to squash until everything's approved?), so I'm trusting you.

@FrankYFTang FrankYFTang merged commit 6ba5a1a into unicode-org:main Aug 3, 2023
101 checks passed
@FrankYFTang FrankYFTang deleted the ICU-22365-C-LocaleBuilder branch August 3, 2023 21:11
catamorphism pushed a commit to catamorphism/icu that referenced this pull request Nov 1, 2023
@FrankYFTang @catamorphism
ICU-22365 C API for ULocaleBuilder
54662e6 
See unicode-org#2520

ICU-22365 Fix comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusicu markusicu markusicu left review comments

@richgillam richgillam richgillam approved these changes

@garywade garywade Awaiting requested review from garywade

Assignees

@richgillam richgillam

Labels
api Adds or changes public API. waiting-on-reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FrankYFTang @markusicu @richgillam