CLDR-17850 Show when patterns allow different widths or not #3997
Conversation
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
| @@ -169,6 +171,7 @@ public class ExampleGenerator { | |||
| private static final Date DATE_SAMPLE2; | |||
| private static final Date DATE_SAMPLE3; | |||
| private static final Date DATE_SAMPLE4; | |||
| private static final Date DATE_SAMPLE5; | |||
There was a problem hiding this comment.
This is redundant with CLDR-15698 #3895, which is ready for merge
| break; | ||
| } | ||
| } else { | ||
| altPatternBuilder.append(item); |
There was a problem hiding this comment.
If I understand correctly, if item is not a VariableField, it's a literal string. The ticket description mentions using fp.toString(i, i + 1). Is this acceptable in its place?
There was a problem hiding this comment.
You want to leave the literals completely alone. So the above looks good.
| + "\"]"; | ||
| String forcedAltValue = cldrFile.getWinningValue(altXpath); | ||
| String unforcedAltValue = generateAltPattern(value); | ||
| String forcePrefix = forcedAltValue != null ? "" : EXAMPLE_OF_INCORRECT; |
There was a problem hiding this comment.
Originally, I had the condition as forcedAltValue.equals(unforcedAltValue)
That didn't work (forcedAltValue was sometimes null), so I tried this instead. Does this make sense?
There was a problem hiding this comment.
If it does make sense, I realize I've flipped the condition (forcing would be when the value is not null, not when the value is null)
However, I'm very dubious of this, since it's not what we discussed. As written, it seems like forcedAltValue is always null.
| + "']/timeSeparator"); | ||
| dfs = sdf.getDateFormatSymbols(); | ||
| dfs.setTimeSeparatorString(timeSeparator); | ||
| sdf.setDateFormatSymbols(dfs); |
There was a problem hiding this comment.
I copy-pasted this chunk from above. The only change is icuServiceBuilder.getDateFormat(calendar, unforcedAltValue, numbersOverride); instead of icuServiceBuilder.getDateFormat(calendar, value, numbersOverride); Is there a way to just change the pattern on a SimpleDateFormat instead of creating a new one?
There was a problem hiding this comment.
You could pull that chunk of code out into a function, and call with slightly different parameters. Better that code copying (which is harder to maintain).
|
|
||
| example = addExampleResult(forcePrefix + sdf.format(DATE_SAMPLE) + ", " + sdf.format(DATE_SAMPLE5), example, showContexts); | ||
| } | ||
| return example; |
There was a problem hiding this comment.
Should I account for the details checked below, i.e., show context if value does not contain "MMM" or "LLL" ?
| @@ -2411,6 +2418,36 @@ private String handleTimeZoneName(XPathParts parts, String value) { | |||
| return result; | |||
| } | |||
|
|
|||
| private String generateAltPattern(String dateTimeValue) { | |||
There was a problem hiding this comment.
Rereading the ticket, I see "the caller can specify the width of various fields." Right now, I'm only accounting for different widths of hours (I thought, at first, that only hours were relevant). Should I be checking every variable field? In that case, couldn't the number of possible examples be very high (e.g. if all of h,m,s can have their width changed, would 8 examples be desired)?
There was a problem hiding this comment.
For time, we only allow the first to be padded or not, eg:
9:08:07 or 09:08:07, but not 9:8:7
There was a problem hiding this comment.
Add comment that this function could be generalizes for fields other than h/H. (If you had more time, you could do it, but the hourglass has run out....)
CLDR-17850
ALLOW_MANY_COMMITS=true