Skip to content

Commit

Permalink
CLDR-17289 lazy setCldrFileToCheck, first pass of adding accept() calls
Browse files Browse the repository at this point in the history
  • Loading branch information
srl295 committed Dec 19, 2023
1 parent e2f2626 commit b8715a9
Show file tree
Hide file tree
Showing 27 changed files with 111 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public CheckCLDR handleCheck(
return this;
}

if (!accept(result)) return this;

String strippedPath = CLDRFile.getNondraftNonaltXPath(path);
if (strippedPath.equals(path)) {
return this; // paths equal, skip
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public CheckCLDR handleCheck(
if (file.getConstructedValue(nonAltPath) != null) {
return this;
}
if (!accept(result)) return this;
/*
* If the source locale is not code-fallback, it's not an error.
* getSourceLocaleIdExtended with skipInheritanceMarker = false means that if the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public CheckCLDR handleCheck(
|| !getCldrFileToCheck().isNotRoot(path)) {
return this;
}
if (!accept(result)) return this;
final String ecode = hasAnnotationECode(value);

if (ecode != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public CheckCLDR handleCheck(
if (!getCldrFileToCheck().getLocaleID().equals(locale)) {
return this;
}
if (!accept(result)) return this;
XPathParts parts = XPathParts.getFrozenInstance(fullPath);
for (int i = 0; i < parts.size(); ++i) {
if (parts.getAttributeCount(i) == 0) {
Expand Down
94 changes: 78 additions & 16 deletions tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ public Options() {
public Options(Options options2) {
this.options = Arrays.copyOf(options2.options, options2.options.length);
this.key = options2.key;
this.locale = options2.locale;
}

public Options(
Expand Down Expand Up @@ -650,17 +651,39 @@ public final CLDRFile getCldrFileToCheck() {
}

/**
* Set the CLDRFile. Must be done before calling check. If null is called, just skip Often
* subclassed for initializing. If so, make the first 2 lines: if (cldrFileToCheck == null)
* return this; super.handleSetCldrFileToCheck(cldrFileToCheck); do stuff
* Often subclassed for initializing. If so, make the first 2 lines: if (cldrFileToCheck ==
* null) return this; super.handleSetCldrFileToCheck(cldrFileToCheck); do stuff
*
* <p>Called late via accept().
*
* @param cldrFileToCheck
* @param options (not currently used)
* @param possibleErrors TODO
* @param options
* @param possibleErrors any deferred possibleErrors can be set here. They will be appended to
* every handleCheck() call.
* @return
*/
public CheckCLDR handleSetCldrFileToCheck(
CLDRFile cldrFileToCheck, Options options, List<CheckStatus> possibleErrors) {

// nothing by default
return this;
}

/**
* Set the CLDRFile. Must be done before calling check.
*
* @param cldrFileToCheck
* @param options (not currently used)
* @param possibleErrors
*/
public CheckCLDR setCldrFileToCheck(
CLDRFile cldrFileToCheck, Options options, List<CheckStatus> possibleErrors) {
this.cldrFileToCheck = cldrFileToCheck;
initted = false;
// clear the *cached* possible Errors. Not counting any set immediately by subclasses.
cachedPossibleErrors.clear();
cachedOptions = new Options(options);
// we must load filters here, as they are used by check()

// Shortlist error filters for this locale.
loadFilters();
Expand All @@ -675,14 +698,52 @@ public CheckCLDR handleSetCldrFileToCheck(
}
xpaths.add(filter.get2());
}

// hook for checks that want to set possibleErrors early
handleCheckPossibleErrors(cldrFileToCheck, options, possibleErrors);

return this;
}

public CheckCLDR setCldrFileToCheck(
protected void handleCheckPossibleErrors(
CLDRFile cldrFileToCheck, Options options, List<CheckStatus> possibleErrors) {
return handleSetCldrFileToCheck(cldrFileToCheck, options, possibleErrors);
// nothing by default.
}

/**
* Subclasses must call this, after any skip calculation to indicate that an xpath is relevant
* to them.
*
* @param result out-parameter to contain any deferred errors
* @return false if test is skipped and should exit
*/
protected boolean accept(List<CheckStatus> result) {
if (!initted) {
if (this.cldrFileToCheck == null) {
throw new NullPointerException("accept() was called before setCldrFileToCheck()");
}
// clear this again.
cachedPossibleErrors.clear();
// call into the subclass
handleSetCldrFileToCheck(this.cldrFileToCheck, cachedOptions, cachedPossibleErrors);
initted = true;
}
// unconditionally append all cached possible errors
result.addAll(cachedPossibleErrors);
if (isSkipTest()) {
return false;
}
return true;
}

/** has accept() been called since setCldrFileToCheck() was called? */
boolean initted = false;

/** cache of possible errors, for handleSetCldrFileToCheck */
List<CheckStatus> cachedPossibleErrors = new ArrayList<>();

Options cachedOptions = null;

/** Status value returned from check */
public static class CheckStatus {
public static final Type alertType = Type.Comment,
Expand Down Expand Up @@ -1212,9 +1273,10 @@ protected CheckCLDR handleGetExamples(
}

/**
* This is what the subclasses override. If they ever use pathParts or fullPathParts, they need
* to call initialize() with the respective path. Otherwise they must NOT change pathParts or
* fullPathParts.
* This is what the subclasses override.
*
* <p>If a path is not applicable, exit early with <code>return this;</code> Once a path is
* applicable, call <code>accept(result);</code> to add deferred possible problems.
*
* <p>If something is found, a CheckStatus is added to result. This can be done multiple times
* in one call, if multiple errors or warnings are found. The CheckStatus may return warnings,
Expand All @@ -1227,8 +1289,6 @@ protected CheckCLDR handleGetExamples(
* .setType(CheckStatus.errorType)
* .setMessage(&quot;Value should be {0}&quot;, new Object[] { pattern }));
* </pre>
*
* @param options TODO
*/
public abstract CheckCLDR handleCheck(
String path, String fullPath, String value, Options options, List<CheckStatus> result);
Expand Down Expand Up @@ -1267,6 +1327,9 @@ public CheckCLDR handleCheck(
Options options,
List<CheckStatus> result) {
result.clear();

if (!accept(result)) return this;

// If we're being asked to run tests for an inheritance marker, then we need to change
// it
// to the "real" value first before running tests. Testing the value
Expand Down Expand Up @@ -1339,15 +1402,15 @@ private void addError(List<CheckStatus> result, CheckCLDR item, Exception e) {
}

@Override
public CheckCLDR handleSetCldrFileToCheck(
public void handleCheckPossibleErrors(
CLDRFile cldrFileToCheck, Options options, List<CheckStatus> possibleErrors) {
ElapsedTimer testTime = null, testOverallTime = null;
if (cldrFileToCheck == null) return this;
if (cldrFileToCheck == null) return;
boolean SHOW_TIMES = options.contains(Options.Option.SHOW_TIMES);
setPhase(Phase.forString(options.get(Options.Option.phase)));
if (SHOW_TIMES)
testOverallTime = new ElapsedTimer("Test setup time for setCldrFileToCheck: {0}");
super.handleSetCldrFileToCheck(cldrFileToCheck, options, possibleErrors);
super.handleCheckPossibleErrors(cldrFileToCheck, options, possibleErrors);
possibleErrors.clear();

for (Iterator<CheckCLDR> it = filteredCheckList.iterator(); it.hasNext(); ) {
Expand All @@ -1372,7 +1435,6 @@ public CheckCLDR handleSetCldrFileToCheck(
}
}
if (SHOW_TIMES) System.out.println("Overall: " + testOverallTime + ": {0}");
return this;
}

public Matcher getFilter() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public CheckCLDR handleCheck(
if (fullPath == null) return this; // skip paths that we don't have
if (fullPath.indexOf("casing") < 0) return this;

if (!accept(result)) return this;
// pick up the casing attributes from the full path
XPathParts parts = XPathParts.getFrozenInstance(fullPath);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,13 @@ public CheckChildren(Factory factory) {
public CheckCLDR handleCheck(
String path, String fullPath, String value, Options options, List<CheckStatus> result) {
if (immediateChildren == null) return this; // skip - test isn't even relevant
if (isSkipTest()) return this; // disabled
if (fullPath == null) return this; // skip paths that we don't have
if (value == null) return this; // skip null values
String winningValue = this.getCldrFileToCheck().getWinningValue(fullPath);
if (!value.equals(winningValue)) {
return this; // only run this test against winning values.
}

if (!accept(result)) return this;
// String current = getResolvedCldrFileToCheck().getStringValue(path);
tempSet.clear();
for (int i = 0; i < immediateChildren.length; ++i) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public CheckCLDR handleCheck(
String path, String fullPath, String value, Options options, List<CheckStatus> result) {
// it helps performance to have a quick reject of most paths
if (fullPath == null) return this; // skip paths that we don't have
if (!accept(result)) return this; // causes hasCasingInfo to be calculated
if (!hasCasingInfo) return this;

String locale2 = getCldrFileToCheck().getSourceLocaleID(path, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ public CheckCoverage(Factory factory) {
public CheckCLDR handleCheck(
String path, String fullPath, String value, Options options, List<CheckStatus> result) {

if (isSkipTest()) {
return this;
}
if (!accept(result)) return this;

CLDRFile resolvedCldrFileToCheck = getResolvedCldrFileToCheck();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public CheckCLDR handleCheck(
// it helps performance to have a quick reject of most paths
if (fullPath == null) return this; // skip paths that we don't have
if (path.indexOf("/currency") < 0 || path.indexOf("/symbol") < 0) return this;

if (!accept(result)) return this;
// parts.set(path); // normally you have to parse out a path to get the exact one, but in
// this case the quick
// reject suffices
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,8 @@ public CheckCLDR handleCheck(
return this;
}

if (!accept(result)) return this;

String sourceLocale = getCldrFileToCheck().getSourceLocaleID(path, status);

if (!path.equals(status.pathWhereFound)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ public CheckCLDR handleCheck(
|| value.equals(CldrUtility.INHERITANCE_MARKER)) {
return this;
}
if (!accept(result)) return this;

// find my type; bail if I don't have one.
Type myType = Type.getType(path);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ public CheckCLDR handleCheck(
}
return this;
}
if (!accept(result)) return this;
XPathParts oparts = XPathParts.getFrozenInstance(path);
final String exemplarString = oparts.findAttributeValue("exemplarCharacters", "type");
ExemplarType type =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public CheckCLDR handleCheck(
if (fullPath == null || path == null || value == null) {
return this; // skip root, and paths that we don't have
}
if (!accept(result)) return this;
Failure failure =
sameAsCodeOrEnglish(value, path, unresolvedFile, getCldrFileToCheck(), false);
addFailure(result, failure);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ public CheckCLDR handleCheck(
String path, String fullPath, String value, Options options, List<CheckStatus> result) {
if (fullPath == null) return this; // skip paths that we don't have
if (value == null) return this; // skip values that we don't have ?
if (!accept(result)) return this;
if (skip) return this;
if (path == null) {
throw new InternalCldrException("Empty path!");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public CheckCLDR handleCheck(
if (value == null) {
return this;
}
if (!accept(result)) return this;

if (value.contains(CldrUtility.INHERITANCE_MARKER)) {
result.add(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public CheckCLDR handleCheck(
if (LogicalGrouping.isOptional(getCldrFileToCheck(), path)) {
return this;
}
if (!accept(result)) return this;
new LogicalGroupChecker(this, path, value, result).run();
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public CheckCLDR handleCheck(
if (fullPath == null) return this; // skip paths that we don't have
if (value == null) return this; // skip empty values
if (path.indexOf("/metazone") < 0) return this;
if (!accept(result)) return this;

// we're simply going to test to make sure that metazone values don't contain any digits
if (value.matches(".*\\p{Nd}.*")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public CheckCLDR handleCheck(
if (!YEARS_NOT_ALLOWED.matcher(path).matches() || !getCldrFileToCheck().isNotRoot(path)) {
return this;
}
if (!accept(result)) return this;
Matcher matcher = RegexUtilities.PATTERN_3_OR_4_DIGITS.matcher(value);
if (matcher.find()) {
// If same as the code-fallback value (territories) then no error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public CheckCLDR handleCheck(
CLDRFile cldrFileToCheck = getCldrFileToCheck();
// don't check inherited values
// first see if the value is inherited or not
if (!accept(result)) return this;
if (!isRoot
&& value != null
&& AnnotationUtil.pathIsAnnotation(path)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ public CheckCLDR handleCheck(

if (fullPath == null || value == null) return this; // skip paths that we don't have

// TODO: could exclude more paths
if (!accept(result)) return this;

// Do a quick check on the currencyMatch, to make sure that it is a proper UnicodeSet
if (path.indexOf("/currencyMatch") >= 0) {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public CheckCLDR handleCheck(
if (isRoot || !path.startsWith("//ldml/personNames/")) {
return this;
}
if (!accept(result)) return this;

XPathParts parts = XPathParts.getFrozenInstance(path);
switch (parts.getElement(2)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ public CheckCLDR handleCheck(
if (value == null || path.endsWith("/alias") || SKIP_PATH_LIST.matcher(path).matches()) {
return this;
}
// TODO: more skips here
if (!accept(result)) return this;

if (path.contains("/personNames")) {
XPathParts parts = XPathParts.getFrozenInstance(path);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public CheckCLDR handleCheck(
}

if (UNITS.matcher(path).matches()) {
if (!accept(result)) return this;
Matcher matcher = ASCII_QUOTES.matcher(value);
CheckStatus.Type type = CheckStatus.warningType;
if (this.getCldrFileToCheck().getLocaleID().equals("en")) {
Expand All @@ -47,6 +48,7 @@ public CheckCLDR handleCheck(
}
}
if (DELIMITERS.matcher(path).matches()) {
if (!accept(result)) return this;
if (!VALID_DELIMITERS.contains(value)) {
result.add(
new CheckStatus()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public CheckCLDR handleCheck(
if (value == null || !path.startsWith("//ldml/units")) {
return this;
}
if (!accept(result)) return this;
final XPathParts parts = XPathParts.getFrozenInstance(path);
String finalElement = parts.getElement(-1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,7 @@ public CheckCLDR handleCheck(
if (value == null) {
return this; // skip
}
if (!accept(result)) return this;
// String testPrefix = "//ldml/units/unitLength[@type=\"narrow\"]";
// if (path.startsWith(testPrefix)) {
// int i = 0;
Expand Down
Loading

0 comments on commit b8715a9

Please sign in to comment.