From a62ef81017e4fa927b7406a770afa5dfde1fcad2 Mon Sep 17 00:00:00 2001 From: Peter Edberg <42151464+pedberg-icu@users.noreply.github.com> Date: Thu, 16 Sep 2021 11:54:26 -0700 Subject: [PATCH] CLDR-14877 Coverage Progress Bars (#1469) (#1502) -Remove superfluous path exclusion checks for altProposed, /references -Refactor and format DataSection.ensureComplete to clarify it only applies to timeZoneNames paths -Use ph.shouldHide instead of SurveyToolStatus.DEPRECATED/HIDE where the distinction is unneeded -Simplify code that had impossible conditions in TestPathHeader -Fix deprecation warnings for SurveyLog, setCldrFileToCheck -Comments, clean-up (cherry picked from commit 56ca5d563cf57990a7598f570cb9be51956cb9de) Co-authored-by: Tom Bishop --- .../org/unicode/cldr/web/DataSection.java | 199 +++++++++--------- .../java/org/unicode/cldr/test/CheckCLDR.java | 14 +- .../cldr/tool/GenerateSidewaysView.java | 2 +- .../org/unicode/cldr/util/VettingViewer.java | 35 +-- .../unicode/cldr/unittest/TestCheckCLDR.java | 7 +- .../unicode/cldr/unittest/TestPathHeader.java | 38 +--- 6 files changed, 120 insertions(+), 175 deletions(-) diff --git a/tools/cldr-apps/src/main/java/org/unicode/cldr/web/DataSection.java b/tools/cldr-apps/src/main/java/org/unicode/cldr/web/DataSection.java index 6937553e8e0..3d1ce52dd8f 100644 --- a/tools/cldr-apps/src/main/java/org/unicode/cldr/web/DataSection.java +++ b/tools/cldr-apps/src/main/java/org/unicode/cldr/web/DataSection.java @@ -22,6 +22,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import java.util.logging.Logger; import java.util.regex.Pattern; import org.json.JSONArray; @@ -76,6 +77,8 @@ */ public class DataSection implements JSONString { + private final static Logger logger = SurveyLog.forClass(DataSection.class); + /* * For debugging only (so far), setting USE_CANDIDATE_HISTORY to true causes * a "history" to be constructed and passed to the client for each CandidateItem, @@ -212,9 +215,8 @@ public String getProcessedValue() { try { return getProcessor().processForDisplay(xpath, rawValue); } catch (Throwable t) { - if (SurveyLog.DEBUG) { - SurveyLog.logException(t, "While processing " + xpath + ":" + rawValue); - } + String msg = "getProcessedValue, while processing " + xpath + ":" + rawValue; + logger.log(java.util.logging.Level.FINE, msg, t); return rawValue; } } @@ -1773,119 +1775,119 @@ public DisplaySet createDisplaySet(SortMode sortMode, XPathMatcher matcher) { } /** - * Makes sure this DataSection contains the rows we'd like to see. + * Makes sure this DataSection contains the rows we'd like to see related to timeZoneNames * * Called only by DataSection.make, only when pageId == null + * + * TODO: explain this mechanism and why it's limited to DataSection, not shared with Dashboard, + * CLDRFile, or any other module. Is it in any way related to CLDRFile.getRawExtraPathsPrivate? */ private void ensureComplete(CLDRFile ourSrc, TestResultBundle checkCldr) { - + if (!xpathPrefix.startsWith("//ldml/dates/timeZoneNames")) { + return; + } STFactory stf = sm.getSTFactory(); - - if (xpathPrefix.startsWith("//ldml/dates/timeZoneNames")) { - // work on zones - boolean isMetazones = xpathPrefix.startsWith("//ldml/dates/timeZoneNames/metazone"); - boolean isSingleXPath = false; - // Make sure the DataSection contains the rows we'd like to see. - // regular zone - - Set zoneIterator; - String podBase = xpathPrefix; - - if (isMetazones) { - if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) { - String[] pieces = xpathPrefix.split(CONTINENT_DIVIDER, 2); - xpathPrefix = pieces[0]; - zoneIterator = sm.getMetazones(pieces[1]); - } else { // This is just a single metazone from a zoom-in - Set singleZone = new HashSet<>(); - XPathParts xpp = XPathParts.getFrozenInstance(xpathPrefix); - String singleMetazoneName = xpp.findAttributeValue("metazone", "type"); - if (singleMetazoneName == null) { - throw new NullPointerException("singleMetazoneName is null for xpp:" + xpathPrefix); - } - singleZone.add(singleMetazoneName); - zoneIterator = singleZone; - - isSingleXPath = true; - } - podBase = "//ldml/dates/timeZoneNames/metazone"; - } else { - if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) { - throw new InternalError("Error: CONTINENT_DIVIDER found on non-metazone xpath " + xpathPrefix); + // work on zones + boolean isMetazones = xpathPrefix.startsWith("//ldml/dates/timeZoneNames/metazone"); + boolean isSingleXPath = false; + // Make sure the DataSection contains the rows we'd like to see. + // regular zone + + Set zoneIterator; + String podBase = xpathPrefix; + + if (isMetazones) { + if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) { + String[] pieces = xpathPrefix.split(CONTINENT_DIVIDER, 2); + xpathPrefix = pieces[0]; + zoneIterator = sm.getMetazones(pieces[1]); + } else { // This is just a single metazone from a zoom-in + Set singleZone = new HashSet<>(); + XPathParts xpp = XPathParts.getFrozenInstance(xpathPrefix); + String singleMetazoneName = xpp.findAttributeValue("metazone", "type"); + if (singleMetazoneName == null) { + throw new NullPointerException("singleMetazoneName is null for xpp:" + xpathPrefix); } - zoneIterator = StandardCodes.make().getGoodAvailableCodes("tzid"); - podBase = "//ldml/dates/timeZoneNames/zone"; - } - if (!isSingleXPath && xpathPrefix.contains("@type")) { + singleZone.add(singleMetazoneName); + zoneIterator = singleZone; + isSingleXPath = true; } + podBase = "//ldml/dates/timeZoneNames/metazone"; + } else { + if (xpathPrefix.indexOf(CONTINENT_DIVIDER) > 0) { + throw new InternalError("Error: CONTINENT_DIVIDER found on non-metazone xpath " + xpathPrefix); + } + zoneIterator = StandardCodes.make().getGoodAvailableCodes("tzid"); + podBase = "//ldml/dates/timeZoneNames/zone"; + } + if (!isSingleXPath && xpathPrefix.contains("@type")) { + isSingleXPath = true; + } - final String tzsuffs[] = { - "/exemplarCity" }; - final String mzsuffs[] = { "/long/generic", "/long/daylight", "/long/standard", "/short/generic", "/short/daylight", - "/short/standard" }; + final String tzsuffs[] = { + "/exemplarCity" }; + final String mzsuffs[] = { "/long/generic", "/long/daylight", "/long/standard", "/short/generic", "/short/daylight", + "/short/standard" }; - String suffs[]; - if (isMetazones) { - suffs = mzsuffs; - } else { - suffs = tzsuffs; + String suffs[]; + if (isMetazones) { + suffs = mzsuffs; + } else { + suffs = tzsuffs; + } + + for (String zone : zoneIterator) { + if (zone == null) { + throw new NullPointerException("zoneIterator.next() returned null! zoneIterator.size: " + zoneIterator.size() + + ", isEmpty: " + zoneIterator.isEmpty()); } + /** some compatibility **/ + String ourSuffix = "[@type=\"" + zone + "\"]"; - CLDRFile resolvedFile = ourSrc; + for (int i = 0; i < suffs.length; i++) { + String suff = suffs[i]; - for (String zone : zoneIterator) { - if (zone == null) { - throw new NullPointerException("zoneIterator.next() returned null! zoneIterator.size: " + zoneIterator.size() - + ", isEmpty: " + zoneIterator.isEmpty()); + if (isSingleXPath && !xpathPrefix.contains(suff)) { + continue; // Try not to add paths that won't be shown. } - /** some compatibility **/ - String ourSuffix = "[@type=\"" + zone + "\"]"; - - for (int i = 0; i < suffs.length; i++) { - String suff = suffs[i]; + // synthesize a new row.. + String base_xpath_string = podBase + ourSuffix + suff; - if (isSingleXPath && !xpathPrefix.contains(suff)) { - continue; // Try not to add paths that won't be shown. - } - // synthesize a new row.. - String base_xpath_string = podBase + ourSuffix + suff; + PathHeader ph = stf.getPathHeader(base_xpath_string); + if (ph == null || ph.shouldHide()) { + continue; + } - PathHeader ph = stf.getPathHeader(base_xpath_string); - if (ph == null || ph.shouldHide()) { - continue; - } + int xpid = sm.xpt.getByXpath(base_xpath_string); + if (matcher != null && !matcher.matches(base_xpath_string, xpid)) { + continue; + } + if (excludeAlways.matcher(base_xpath_string).matches()) { + continue; + } - int xpid = sm.xpt.getByXpath(base_xpath_string); - if (matcher != null && !matcher.matches(base_xpath_string, xpid)) { + // Only display metazone data for which an English value exists + if (isMetazones && suff != "/commonlyUsed") { + String engValue = translationHintsFile.getStringValue(base_xpath_string); + if (engValue == null || engValue.length() == 0) { continue; } - if (excludeAlways.matcher(base_xpath_string).matches()) { - continue; - } - - // Only display metazone data for which an English value exists - if (isMetazones && suff != "/commonlyUsed") { - String engValue = translationHintsFile.getStringValue(base_xpath_string); - if (engValue == null || engValue.length() == 0) { - continue; - } - } - // Filter out data that is higher than the desired coverage level - int coverageValue = getCoverageInfo().getCoverageValue(base_xpath_string, locale.getBaseName()); + } + // Filter out data that is higher than the desired coverage level + int coverageValue = getCoverageInfo().getCoverageValue(base_xpath_string, locale.getBaseName()); - DataSection.DataRow myp = getDataRow(base_xpath_string); /* rowXPath */ + DataSection.DataRow myp = getDataRow(base_xpath_string); /* rowXPath */ - myp.coverageValue = coverageValue; + myp.coverageValue = coverageValue; - // set it up.. - int base_xpath = sm.xpt.getByXpath(base_xpath_string); + // set it up.. + int base_xpath = sm.xpt.getByXpath(base_xpath_string); - // set up tests - myp.setShimTests(base_xpath, base_xpath_string, checkCldr); - } // end inner for loop - } // end outer for loop - } // end if timezone + // set up tests + myp.setShimTests(base_xpath, base_xpath_string, checkCldr); + } // end inner for loop + } // end outer for loop } /** @@ -1939,9 +1941,6 @@ public DataRow getDataRow(String xpath) { * * @param ourSrc the CLDRFile * @param checkCldr the TestResultBundle - * Old param workingCoverageLevel was always "comprehensive" - * - * Called only by DataSection.make, as section.populateFrom(ourSrc, checkCldr, workingCoverageLevel). */ private void populateFrom(CLDRFile ourSrc, TestResultBundle checkCldr) { STFactory stf = sm.getSTFactory(); @@ -2322,8 +2321,9 @@ public String toJSONString() throws JSONException { JSONObject obj = new JSONObject(str); itemList.put(d.fieldHash(), obj); } catch (JSONException ex) { - SurveyLog.logException(ex, "JSON serialization error for row: " - + d.xpath + " : Full row is: " + d.toString()); + String msg = "JSON serialization error for row: " + + d.xpath + " : Full row is: " + d.toString(); + logger.log(java.util.logging.Level.WARNING, msg, ex); throw new JSONException(ex); } } @@ -2331,7 +2331,8 @@ public String toJSONString() throws JSONException { result.put("xpathPrefix", xpathPrefix); return result.toString(); } catch (Throwable t) { - SurveyLog.logException(t, "Trying to load rows for " + this.toString()); + String msg = "Trying to load rows for " + this.toString(); + logger.log(java.util.logging.Level.WARNING, msg, t); throw new JSONException(t); } } diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java b/tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java index 54292fa7d9c..889eddd8879 100644 --- a/tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java +++ b/tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java @@ -169,7 +169,13 @@ public StatusAction getShowRowAction( ) { PathHeader.SurveyToolStatus status = ph.getSurveyToolStatus(); - // always forbid deprecated items - don't show. + /* + * Always forbid DEPRECATED items - don't show. + * + * Currently, bulk submission and TC voting are allowed even for SurveyToolStatus.HIDE, + * but not for SurveyToolStatus.DEPRECATED. If we ever want to treat HIDE and DEPRECATED + * the same here, then it would be simpler to call ph.shouldHide which is true for both. + */ if (status == SurveyToolStatus.DEPRECATED) { return StatusAction.FORBID_READONLY; } @@ -178,12 +184,6 @@ public StatusAction getShowRowAction( return StatusAction.ALLOW_TICKET_ONLY; } - /* - * TODO: is it intentional that bulk submission and TC voting are allowed even for SurveyToolStatus.HIDE? - * If not, fix it by calling PathHeader.shouldHide() above instead of referencing SurveyToolStatus.DEPRECATED - * above and SurveyToolStatus.HIDE below. Otherwise add a comment here confirming that these are allowed for - * SurveyToolStatus.HIDE. Reference: https://unicode-org.atlassian.net/browse/CLDR-14877 - */ // always forbid bulk import except in data submission. if (inputMethod == InputMethod.BULK && this != Phase.SUBMISSION) { diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateSidewaysView.java b/tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateSidewaysView.java index 26e3a374735..eea9f5a212c 100644 --- a/tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateSidewaysView.java +++ b/tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateSidewaysView.java @@ -637,7 +637,7 @@ private static void loadInformation(Factory cldrFactory) { if (path.indexOf("/identity") >= 0) continue; if (path.indexOf("/references") >= 0) continue; PathHeader ph = fixPath(path, postFix); - if (ph.shouldHide()) { + if (ph == null || ph.shouldHide()) { continue; } String fullPath = cldrFile.getFullXPath(path); diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/util/VettingViewer.java b/tools/cldr-code/src/main/java/org/unicode/cldr/util/VettingViewer.java index 9d64106eecb..373964dcdda 100644 --- a/tools/cldr-code/src/main/java/org/unicode/cldr/util/VettingViewer.java +++ b/tools/cldr-code/src/main/java/org/unicode/cldr/util/VettingViewer.java @@ -17,12 +17,12 @@ import java.util.TreeSet; import java.util.concurrent.ForkJoinPool; import java.util.concurrent.RecursiveAction; -import java.util.regex.Matcher; import java.util.regex.Pattern; import org.unicode.cldr.test.CheckCLDR; import org.unicode.cldr.test.CheckCLDR.CheckStatus; import org.unicode.cldr.test.CheckCLDR.CheckStatus.Subtype; +import org.unicode.cldr.test.CheckCLDR.Options; import org.unicode.cldr.test.CheckCoverage; import org.unicode.cldr.test.CheckNew; import org.unicode.cldr.test.CoverageLevel2; @@ -62,8 +62,6 @@ public class VettingViewer { private static final String SPLIT_CHAR = "\uFFFE"; - private static final Pattern ALT_PROPOSED = PatternCache.get("\\[@alt=\"[^\"]*proposed"); - private static final boolean DEBUG_THREADS = false; public static Set OK_IF_VOTED = EnumSet.of(Subtype.sameAsEnglish); @@ -338,7 +336,7 @@ public Status initErrorStatus(CLDRFile cldrFile) { options = new HashMap<>(); result = new ArrayList<>(); checkCldr = CheckCLDR.getCheckAll(factory, ".*"); - checkCldr.setCldrFileToCheck(cldrFile, options, result); + checkCldr.setCldrFileToCheck(cldrFile, new Options(options), result); return Status.ok; } @@ -545,7 +543,6 @@ private FileInfo getFileInfo(CLDRFile sourceFile, CLDRFile baselineFile, final DefaultErrorStatus errorChecker = new DefaultErrorStatus(cldrFactory); errorChecker.initErrorStatus(sourceFile); - Matcher altProposed = ALT_PROPOSED.matcher(""); problems = EnumSet.noneOf(Choice.class); // now look through the paths @@ -557,9 +554,9 @@ private FileInfo getFileInfo(CLDRFile sourceFile, CLDRFile baselineFile, boolean latin = VettingViewer.isLatinScriptLocale(sourceFile); CLDRFile baselineFileUnresolved = (baselineFile == null) ? null : baselineFile.getUnresolved(); for (String path : sourceFile.fullIterable()) { - if (specificSinglePath != null && !specificSinglePath.equals(path)) + if (specificSinglePath != null && !specificSinglePath.equals(path)) { continue; - + } String value = sourceFile.getWinningValueForVettingViewer(path); statusMessage.setLength(0); subtypes.clear(); @@ -572,34 +569,12 @@ private FileInfo getFileInfo(CLDRFile sourceFile, CLDRFile baselineFile, progressCallback.nudge(); // Let the user know we're moving along. PathHeader ph = pathTransform.fromPath(path); - if (ph.shouldHide()) { + if (ph == null || ph.shouldHide()) { continue; } // note that the value might be missing! - // make sure we only look at the real values - if (altProposed.reset(path).find()) { - /* - * TODO: explain this exclusion mechanism and why it's not implemented with - * PathHeader.SurveyToolStatus.HIDE and PathHeader.shouldHide(). - * I put a breakpoint on this “continue” and never hit it. Is this dead code? - * If there are any matching paths, are they getting filtered out earlier for - * some other reason? Does DataSection have or need a similar filter? - * Reference: https://unicode-org.atlassian.net/browse/CLDR-14877 - */ - continue; - } - - /* - * TODO: as above, explain this exclusion mechanism, and encapsulate it better; - * where else do we (or should we) skip such paths? - * Reference: https://unicode-org.atlassian.net/browse/CLDR-14877 - */ - if (path.contains("/references")) { - continue; - } - Level level = supplementalDataInfo.getCoverageLevel(path, sourceFile.getLocaleID()); // skip all but errors above the requested level diff --git a/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCheckCLDR.java b/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCheckCLDR.java index fa75352b92b..8ea59c3db98 100644 --- a/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCheckCLDR.java +++ b/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCheckCLDR.java @@ -777,12 +777,7 @@ public void TestShowRowAction() { ph, dummyUserInfo); - /* - * TODO: if there is a reason to distinguish between SurveyToolStatus.HIDE and - * SurveyToolStatus.DEPRECATED here, explain it with a comment; otherwise, call - * ph.shouldHide() instead. Reference: https://unicode-org.atlassian.net/browse/CLDR-14877 - */ - if (surveyToolStatus == SurveyToolStatus.HIDE) { + if (ph.shouldHide()) { assertEquals("HIDE ==> FORBID_READONLY", StatusAction.FORBID_READONLY, action); } else if (CheckCLDR.LIMITED_SUBMISSION) { if (status == CheckStatus.Type.Error) { diff --git a/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPathHeader.java b/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPathHeader.java index f2c76721997..42483ecdd7e 100644 --- a/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPathHeader.java +++ b/tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPathHeader.java @@ -361,20 +361,15 @@ public void TestOptional() { continue; } - PathHeader p = pathHeaderFactory.fromPath(path); - final SurveyToolStatus status = p.getSurveyToolStatus(); - /* - * TODO: is it intentional that SurveyToolStatus.DEPRECATED is distinguished from - * SurveyToolStatus.HIDE here? If so, add a comment to make the intention clear; otherwise, - * call PathHeader.shouldHide(). Reference: https://unicode-org.atlassian.net/browse/CLDR-14877 - */ - if (status == SurveyToolStatus.DEPRECATED) { + PathHeader ph = pathHeaderFactory.fromPath(path); + if (ph == null || ph.shouldHide()) { continue; } + final SurveyToolStatus status = ph.getSurveyToolStatus(); sorted.put( - p, - locale + "\t" + status + "\t" + p + "\t" - + p.getOriginalPath()); + ph, + locale + "\t" + status + "\t" + ph + "\t" + + ph.getOriginalPath()); } Set codes = new LinkedHashSet<>(); PathHeader old = null; @@ -677,7 +672,6 @@ public void TestStatus() { SurveyToolStatus.class); Set nuked = new HashSet<>(); Set deprecatedStar = new HashSet<>(); - Set differentStar = new HashSet<>(); for (String path : nativeFile.fullIterable()) { @@ -690,32 +684,12 @@ public void TestStatus() { + ": " + p); } - /* - * TODO: if the goal here is to treat SurveyToolStatus.DEPRECATED and SurveyToolStatus.HIDE - * the same for the purpose of this test, then call PathHeader.shouldHide(). Otherwise, add a - * comment to explain this mysterious code. Reference: https://unicode-org.atlassian.net/browse/CLDR-14877 - */ - final SurveyToolStatus tempSTS = surveyToolStatus == SurveyToolStatus.DEPRECATED ? SurveyToolStatus.HIDE - : surveyToolStatus; String starred = starrer.set(path); List attr = starrer.getAttributes(); if (surveyToolStatus != SurveyToolStatus.READ_WRITE) { nuked.add(starred); } - // check against old - SurveyToolStatus oldStatus = SurveyToolStatus.READ_WRITE; - - if (tempSTS != oldStatus - && oldStatus != SurveyToolStatus.READ_WRITE - && !path.endsWith(APPEND_TIMEZONE_END)) { - if (!differentStar.contains(starred)) { - errln("Different from old:\t" + oldStatus + "\tnew:\t" - + surveyToolStatus + "\t" + path); - differentStar.add(starred); - } - } - // check against deprecated boolean isDeprecated = supplemental.isDeprecated(DtdType.ldml, path); if (isDeprecated != (surveyToolStatus == SurveyToolStatus.DEPRECATED)) {