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

CLDR-17850 Show when patterns allow different widths or not #3997

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

CLDR-17850 Show when patterns allow different widths or not #3997

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
815b68e
CLDR-17850 Show when patterns allow different widths or not
e-ikwut Aug 28, 2024
9017a10
minor fixes
e-ikwut Aug 28, 2024
a43f22a
minor fixes
e-ikwut Aug 28, 2024
62578ab
minor fixes
e-ikwut Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 99 additions & 30 deletions tools/cldr-code/src/main/java/org/unicode/cldr/test/ExampleGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,5 @@
package org.unicode.cldr.test;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.ibm.icu.impl.Row.R3;
import com.ibm.icu.impl.Utility;
import com.ibm.icu.impl.number.DecimalQuantity;
import com.ibm.icu.impl.number.DecimalQuantity_DualStorageBCD;
import com.ibm.icu.lang.UCharacter;
import com.ibm.icu.text.BreakIterator;
import com.ibm.icu.text.DateFormat;
import com.ibm.icu.text.DateFormatSymbols;
import com.ibm.icu.text.DateTimePatternGenerator;
import com.ibm.icu.text.DecimalFormat;
import com.ibm.icu.text.DecimalFormatSymbols;
import com.ibm.icu.text.ListFormatter;
import com.ibm.icu.text.MessageFormat;
import com.ibm.icu.text.NumberFormat;
import com.ibm.icu.text.PluralRules;
import com.ibm.icu.text.PluralRules.DecimalQuantitySamples;
import com.ibm.icu.text.PluralRules.DecimalQuantitySamplesRange;
import com.ibm.icu.text.PluralRules.Operand;
import com.ibm.icu.text.PluralRules.SampleType;
import com.ibm.icu.text.SimpleDateFormat;
import com.ibm.icu.text.SimpleFormatter;
import com.ibm.icu.text.UTF16;
import com.ibm.icu.text.UnicodeSet;
import com.ibm.icu.util.Calendar;
import com.ibm.icu.util.Output;
import com.ibm.icu.util.TimeZone;
import com.ibm.icu.util.ULocale;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.text.ChoiceFormat;
Expand All @@ -47,6 +18,7 @@
import java.util.function.Function;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.unicode.cldr.tool.LikelySubtags;
import org.unicode.cldr.util.AnnotationUtil;
import org.unicode.cldr.util.CLDRConfig;
Expand Down Expand Up @@ -91,6 +63,36 @@
import org.unicode.cldr.util.personname.PersonNameFormatter.NamePattern;
import org.unicode.cldr.util.personname.SimpleNameObject;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.ibm.icu.impl.Row.R3;
import com.ibm.icu.impl.Utility;
import com.ibm.icu.impl.number.DecimalQuantity;
import com.ibm.icu.impl.number.DecimalQuantity_DualStorageBCD;
import com.ibm.icu.lang.UCharacter;
import com.ibm.icu.text.BreakIterator;
import com.ibm.icu.text.DateFormat;
import com.ibm.icu.text.DateFormatSymbols;
import com.ibm.icu.text.DateTimePatternGenerator;
import com.ibm.icu.text.DecimalFormat;
import com.ibm.icu.text.DecimalFormatSymbols;
import com.ibm.icu.text.ListFormatter;
import com.ibm.icu.text.MessageFormat;
import com.ibm.icu.text.NumberFormat;
import com.ibm.icu.text.PluralRules;
import com.ibm.icu.text.PluralRules.DecimalQuantitySamples;
import com.ibm.icu.text.PluralRules.DecimalQuantitySamplesRange;
import com.ibm.icu.text.PluralRules.Operand;
import com.ibm.icu.text.PluralRules.SampleType;
import com.ibm.icu.text.SimpleDateFormat;
import com.ibm.icu.text.SimpleFormatter;
import com.ibm.icu.text.UTF16;
import com.ibm.icu.text.UnicodeSet;
import com.ibm.icu.util.Calendar;
import com.ibm.icu.util.Output;
import com.ibm.icu.util.TimeZone;
import com.ibm.icu.util.ULocale;

/**
* Class to generate examples and help messages for the Survey tool (or console version).
*
Expand Down Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant with CLDR-15698 #3895, which is ready for merge


static {
Calendar calendar = Calendar.getInstance(ZONE_SAMPLE, ULocale.ENGLISH);
Expand All @@ -182,8 +185,12 @@ public class ExampleGenerator {
DATE_SAMPLE3 = calendar.getTime();
calendar.set(1999, 8, 5, 23, 0, 0); // 1999-09-05 23:00:00
DATE_SAMPLE4 = calendar.getTime();
calendar.set(1999, 8, 5, 3, 25, 59); // 1999-09-05 03:25:59
DATE_SAMPLE5 = calendar.getTime();
}

public static DateTimePatternGenerator.FormatParser fp = new DateTimePatternGenerator.FormatParser();

static final List<DecimalQuantity> CURRENCY_SAMPLES =
ImmutableList.of(
DecimalQuantity_DualStorageBCD.fromExponentString("1.23"),
Expand Down Expand Up @@ -2411,6 +2418,36 @@ private String handleTimeZoneName(XPathParts parts, String value) {
return result;
}

private String generateAltPattern(String dateTimeValue) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....)

fp.set(dateTimeValue);
StringBuilder altPatternBuilder = new StringBuilder();
for (Object item : fp.getItems()) {
if (item instanceof DateTimePatternGenerator.VariableField) {
DateTimePatternGenerator.VariableField field = (DateTimePatternGenerator.VariableField) item;
switch (field.toString()) {
case "h":
altPatternBuilder.append("hh");
break;
case "H":
altPatternBuilder.append("HH");
break;
case "hh":
altPatternBuilder.append("h");
break;
case "HH":
altPatternBuilder.append("H");
break;
default:
altPatternBuilder.append(field.toString());
break;
}
} else {
altPatternBuilder.append(item);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to leave the literals completely alone. So the above looks good.

}
}
return altPatternBuilder.toString();
}

@SuppressWarnings("deprecation")
private String handleDateFormatItem(String xpath, String value, boolean showContexts) {
// Get here if parts contains "calendar" and either of "pattern", "dateFormatItem"
Expand Down Expand Up @@ -2608,7 +2645,39 @@ private String handleDateFormatItem(String xpath, String value, boolean showCont
DateFormatSymbols dfs = sdf.getDateFormatSymbols();
dfs.setTimeSeparatorString(timeSeparator);
sdf.setDateFormatSymbols(dfs);
if (id == null || id.indexOf('B') < 0) {
if (parts.contains("availableFormats")) {
String example = addExampleResult(sdf.format(DATE_SAMPLE) + ", " + sdf.format(DATE_SAMPLE5), "", showContexts);
String altId = generateAltPattern(id);
if (!altId.equals(id)) {
String altXpath = "//ldml/dates/calendars/calendar[@type=\""
+ calendar
+ "\"]/dateTimeFormats/availableFormats/dateFormatItem[@id=\""
+ altId
+ "\"]";
String forcedAltValue = cldrFile.getWinningValue(altXpath);
String unforcedAltValue = generateAltPattern(value);
String forcePrefix = forcedAltValue != null ? "" : EXAMPLE_OF_INCORRECT;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



sdf =
icuServiceBuilder.getDateFormat(calendar, unforcedAltValue, numbersOverride);
sdf.setTimeZone(ZONE_SAMPLE);
defaultNumberingSystem =
cldrFile.getWinningValue("//ldml/numbers/defaultNumberingSystem");
timeSeparator =
cldrFile.getWinningValue(
"//ldml/numbers/symbols[@numberSystem='"
+ defaultNumberingSystem
+ "']/timeSeparator");
dfs = sdf.getDateFormatSymbols();
dfs.setTimeSeparatorString(timeSeparator);
sdf.setDateFormatSymbols(dfs);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor Author

@e-ikwut e-ikwut Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I account for the details checked below, i.e., show context if value does not contain "MMM" or "LLL" ?

}
else if (id == null || id.indexOf('B') < 0) {
// Standard date/time format, or availableFormat without dayPeriod
if (value.contains("MMM") || value.contains("LLL")) {
// alpha month, do not need context examples
Expand Down