-
Notifications
You must be signed in to change notification settings - Fork 419
CLDR-19106 check for empty datetimeformats #5219
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
Conversation
| throw new IllegalArgumentException("Enhance checks to handle {0}{1}"); | ||
| } | ||
|
|
||
| // TODO: also check the available formats that are for a date |
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.
This is an important TODO. We use these glue patterns to assemble date+time patterns from availableFormats.
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.
Right. At first I wasn't checking for this, and just the short datetime pattern needed to be fixed. But then I realized that it needed to be extended.
| if (path.startsWith(collisionPrefix)) { | ||
| if (parts.containsElement("dateTimeFormats") | ||
| && !parts.containsElement("appendItems")) { | ||
| checkDateTimeFormats(path, parts, value, result); |
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.
Question: this means we're checking all of the datetime glue patterns (short/medium/long/full and standard/atTime), correct?
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.
right.
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.
Hmm,, I had approved this when it was still draft, but somehow that got erased. Re-approving now.
|
@pedberg-icu If you have time to look this over this morning, that would be great. I think it is very low-risk. |
pedberg-icu
left a comment
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
(cherry picked from commit bd31250)
|
The only question I really had was that I thought yue and zh-Hant tend to prefer thin spaces? |
|
I was debating between thin space, ascii space, and fat space (aka full-width). I went with what zh.xml had (ascii). |
(cherry picked from commit bd31250)
CLDR-19106
Add test for {0}{1} datetimeformats that could have digits at the end of {0} merging with digits at the start of {1}. Eg, ...ddHH... resulting in a datetime format like Sept3023AM
Fix other formats that could cause problems.
Note: This is a quick fix for a bad situation. We may be able to allow {0}{1} for some combinations of formats, in the future, but it is much worse to have missing spaces when needed, than extra spaces that are not needed.
This PR completes the ticket.
ALLOW_MANY_COMMITS=true