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-22027 Add Temporal related Calendar API #2274

Merged
merged 1 commit into from
Jan 17, 2023
Merged

ICU-22027 Add Temporal related Calendar API #2274

merged 1 commit into from
Jan 17, 2023

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Dec 17, 2022 •
edited by jira bot
Loading

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22027
  • 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

API proposal
https://docs.google.com/document/d/1UYriEzzExiLhi2RD3zjTsI5UQHv1dXaFqrct7yXNdCA/edit#heading=h.x9obor85vpx9

Design Doc https://docs.google.com/document/d/15ViyC9s0k3VEDwBmAkKxxz4IadZ6QrAIoETkdkF0cVA/edit#

@FrankYFTang FrankYFTang self-assigned this Dec 17, 2022
@FrankYFTang FrankYFTang added incomplete Needs work; do not approve/merge as is. do-not-merge Ready for review but shouldn't be merged yet (useful when ICU is preparing for a release) labels Dec 17, 2022
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/calendar.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang dismissed a stale review via 8adf9ba December 21, 2022 05:15
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/calendar.cpp is different
  • icu4c/source/i18n/cecal.cpp is different
  • icu4c/source/i18n/cecal.h is different
  • icu4c/source/i18n/chnsecal.cpp is different
  • icu4c/source/i18n/chnsecal.h is different
  • icu4c/source/i18n/hebrwcal.cpp is different
  • icu4c/source/i18n/hebrwcal.h is different
  • icu4c/source/i18n/unicode/calendar.h is different
  • icu4c/source/i18n/unicode/ucal.h is different
  • icu4c/source/test/intltest/caltest.cpp is different
  • icu4c/source/test/intltest/caltest.h 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/i18n/calendar.cpp is different
  • icu4c/source/i18n/cecal.cpp is different
  • icu4c/source/i18n/cecal.h is different
  • icu4c/source/i18n/chnsecal.cpp is different
  • icu4c/source/i18n/chnsecal.h is different
  • icu4c/source/i18n/hebrwcal.cpp is different
  • icu4c/source/i18n/hebrwcal.h is different
  • icu4c/source/i18n/islamcal.cpp is different
  • icu4c/source/i18n/islamcal.h is different
  • icu4c/source/i18n/unicode/calendar.h is different
  • icu4c/source/test/intltest/caltest.cpp 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. do-not-merge Ready for review but shouldn't be merged yet (useful when ICU is preparing for a release) labels Dec 22, 2022
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/unicode/calendar.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

sorry, please hold the review, there is still some bug in this PR

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/chnsecal.cpp is different
  • icu4c/source/test/intltest/caltest.cpp is different
  • icu4c/source/test/intltest/incaltst.cpp is now changed in the branch

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/i18n/calendar.cpp is different
  • icu4c/source/i18n/chnsecal.cpp is different
  • icu4c/source/i18n/hebrwcal.cpp 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/intltest/caltest.cpp 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/intltest/caltest.cpp is different
  • icu4c/source/test/intltest/caltest.h 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/intltest/caltest.cpp is different

View Diff Across 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

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/caltest.cpp 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/intltest/caltest.cpp 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/intltest/caltest.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

I have no idea why "adaboost-icu4j-build-and-test (pull_request)" is broken here since I didn't touch any Java code in this PR

richgillam
richgillam previously approved these changes Jan 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.

I got about a third of the way through this before I realized that I'm not really qualified to review this code, but for what it's worth, the structural stuff all makes sense to me and I have no reason to doubt how you're doing the calendrical calculations (although I'd really like another pair of eyes on all this stuff). I did have a few small questions about the documentation, but nothing too earth-shattering.

I didn't actually review the unit tests in detail-- I got lost very quickly. I apologize for this.

icu4c/source/i18n/calendar.cpp Show resolved Hide resolved
icu4c/source/i18n/chnsecal.h Show resolved Hide resolved
icu4c/source/i18n/chnsecal.h Show resolved Hide resolved
icu4c/source/i18n/unicode/calendar.h Outdated Show resolved Hide resolved
icu4c/source/i18n/unicode/calendar.h Outdated Show resolved Hide resolved
icu4c/source/i18n/unicode/calendar.h Outdated Show resolved Hide resolved
icu4c/source/test/intltest/caltest.cpp Show resolved Hide resolved
@FrankYFTang FrankYFTang dismissed stale reviews from richgillam and ghost via f044315 January 13, 2023 22:09
@FrankYFTang
Copy link
Contributor Author

Please take another look

richgillam
richgillam previously approved these changes Jan 13, 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.

Still LGTM.

@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 FrankYFTang removed the request for review from markusicu January 17, 2023 17:24
@FrankYFTang
Copy link
Contributor Author

Rich- I squeshed PTAL
Peter- any feedback ?

richgillam
richgillam approved these changes Jan 17, 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.

Still LGTM.

@FrankYFTang FrankYFTang merged commit cd1b772 into unicode-org:main Jan 17, 2023
@FrankYFTang FrankYFTang deleted the ICU-22027-CalAPI branch January 17, 2023 23:08
@FrankYFTang
Copy link
Contributor Author

Peter, if you have other concerns, please file bug against me and I will fix it in subsequent PR. Thanks

@FrankYFTang FrankYFTang mentioned this pull request Jan 31, 2023
Open
7 tasks
@FrankYFTang FrankYFTang mentioned this pull request Jul 28, 2023
Merged
7 tasks
FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Sep 19, 2023
@FrankYFTang
ICU-22512 Fix broken TestHebrewCalendarInTemporalLeapYear
b6b3e89 
Fix broken test mistakenly landed in
unicode-org#2274

Some important steps were missed in the last landing.
@FrankYFTang FrankYFTang mentioned this pull request Sep 19, 2023
Merged
7 tasks
FrankYFTang added a commit that referenced this pull request Sep 19, 2023
@FrankYFTang
ICU-22512 Fix broken TestHebrewCalendarInTemporalLeapYear
f3b869c 
Fix broken test mistakenly landed in
#2274

Some important steps were missed in the last landing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richgillam richgillam richgillam approved these changes

@pedberg-icu pedberg-icu Awaiting requested review from pedberg-icu

Assignees

@FrankYFTang FrankYFTang

Labels
waiting-on-reviewer
Projects
None yet
Milestone
No milestone
Development

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