ICU-23210 Try fixing behavior of MATCH_*_FIELD_LENGTH#3642
ICU-23210 Try fixing behavior of MATCH_*_FIELD_LENGTH#3642sffc wants to merge 1 commit intounicode-org:mainfrom
Conversation
richgillam
left a comment
richgillam
left a comment
There was a problem hiding this comment.
I realized halfway through the review that you're only changing the Java code. I'm guessing you're making it consistent with the C++ code? If so, I'm on board with these changes.
| || (type == MINUTE && (options & MATCH_MINUTE_FIELD_LENGTH)==0) | ||
| || (type == SECOND && (options & MATCH_SECOND_FIELD_LENGTH)==0)) { | ||
| adjFieldLen = fieldBuilder.length(); | ||
| } |
There was a problem hiding this comment.
What's the extra test getting you here? I'm not seeing how that changes the logic, but it must, since the unit-test results are different. What am I missing here?
There was a problem hiding this comment.
The else if had previously been reachable my hour/minute/second, and it was causing the field widths to revert to the pattern widths. With this change, the else if is no longer reachable on those fields.
There was a problem hiding this comment.
Ah, that's what I was missing. Thanks for the clarification.
| {"zh-TW", "CCCCCm", "BBBBBh:mm"}, | ||
| {"zh-TW", "CCCCCCm", "BBBBBhh:mm"}, | ||
| {"de", "Cm", "HH:mm"}, | ||
| {"de", "Cm", "H:mm"}, |
There was a problem hiding this comment.
I notice you're only changing the Java code. The C++ code already did this?
There was a problem hiding this comment.
I figured I would start in ICU4J and continue to ICU4C once I got alignment. (The PR is Draft and I am seeking feedback.)
There was a problem hiding this comment.
That's fine, but it means I shouldn't have approved the PR yet. I can't figure out how to take that back without just blocking the PR (which I've always thought was kind of rude), so I'll trust you. :-)
| new TestOptionsItem( "en", "HHmm", "HH:mm", DateTimePatternGenerator.MATCH_NO_OPTIONS ), | ||
| new TestOptionsItem( "en", "hhmm", "h:mm\u202Fa", DateTimePatternGenerator.MATCH_NO_OPTIONS ), | ||
| new TestOptionsItem( "en", "Hmm", "HH:mm", DateTimePatternGenerator.MATCH_HOUR_FIELD_LENGTH ), | ||
| new TestOptionsItem( "en", "Hmm", "H:mm", DateTimePatternGenerator.MATCH_HOUR_FIELD_LENGTH ), |
There was a problem hiding this comment.
Does this match what the C++ code does? And is there already a test somewhere that shows we still get two digits without MATCH_HOUR_FIELD_LENGTH?
richgillam
left a comment
richgillam
left a comment
There was a problem hiding this comment.
Looks like I approved prematurely.
|
Also please note that I haven't updated all of the ICU4J tests yet. I wanted to ask about a few because @richgillam you recently (1 yr ago) added them in ICU-22669 |
2 participants
Proof-of-concept for ICU-23210
Checklist