-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
ICU-22407 Implement Java Temporal Calendar API #2526
Conversation
87336d2
to
319cf65
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
adc8b52
to
1e84249
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
605e78f
to
667e848
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
80102e5
to
26637c0
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
c844bd7
to
6864c08
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
Since richgillam reviewed #2274 it probably would be easier for him to be the reviewer since it is a 1:1 porting from the C++ implementation |
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 probably should have read this more closely, but I'm running out of brain cells...
This looks good, and it looks like what I remember from the C side.
icu4c/source/i18n/hebrwcal.cpp
Outdated
@@ -777,7 +777,7 @@ int32_t HebrewCalendar::internalGetMonth() const { | |||
HebrewCalendar *nonConstThis = (HebrewCalendar*)this; // cast away const | |||
|
|||
int32_t year = nonConstThis->handleGetExtendedYear(); | |||
return ordinalMonth + ((isLeapYear(year) && (ordinalMonth > ADAR_1)) ? 1: 0); | |||
return ordinalMonth + (((!isLeapYear(year)) && (ordinalMonth > ADAR_1)) ? 1: 0); |
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 take it the lack of !
was a bug, but is the extra pair of parentheses buying you anything?
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.
just make it more explicit that the ! is not applying to A&&B but only to A
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.
and the cpp part is already merged in other PR and removed from this PR
* @draft ICU 74 | ||
*/ | ||
public String getTemporalMonthCode() { | ||
if (get(MONTH) == 12) return "M13"; |
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.
Dumb question, but why isn't "M13"
listed in gTemporalMonthCodes
above?
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.
good catch, the gTemporalMonthCodes should not be here.
6864c08
to
2abbe09
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
PTAL |
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. Thanks for the fixes!
6f379e0
to
247885a
Compare
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
Have post-merge breakage on the following three tests Testcase: TestConsistencyChinese took 10.636 sec Testcase: TestConsistencyDangi took 9.44 sec Testcase: TestConsistencyIndian took 1.273 sec |
The post merge breakage are the same as the C++ one in https://unicode-org.atlassian.net/browse/ICU-22252 |
Checklist