CLDR-18217 Remove Iterable interface from CLDRFile, add iteratorDefault, iterableDefault #4285
Conversation
…lt, iterableDefault -Instead of Iterable interface, use new methods iterableDefault, iterableWithoutExtras -Private boolean CLDRFile.DEFAULT_ITERATION_INCLUDES_EXTRAS determines whether iteratorDefault and iterableDefault skip extras; either way they skip paths with null values -Replace original iterator methods (zero or more parameters) with iteratorDefault or iteratorWithoutExtras -Iterable and iterator are distinct, as in iterableWithoutExtras versus iteratorWithoutExtras -Call iteratorWithoutExtras/iterableWithoutExtras directly where tests would otherwise fail with DEFAULT_ITERATION_INCLUDES_EXTRAS true
|
This goes further than #4280 |
| @@ -105,7 +105,7 @@ private static void showEnglish(PrintWriter out) throws IOException { | |||
| String locale = "en"; | |||
| Set<String> sorted = | |||
| Builder.with(new TreeSet<String>()) | |||
| .addAll(cldrFile.iterator()) | |||
| .addAll(cldrFile.iteratorDefault()) | |||
| .addAll(cldrFile.getExtraPaths()) | |||
There was a problem hiding this comment.
note: if iteratorDefault includes extra paths, then getExtraPaths would be superfluous here, except that iteratorDefault skips paths with null values (getStringValue(path) == null)
alternatively fullIterator() could be called here
|
An essential difference from #4280 is that here, iteration always skips paths with null values (except for fullIterable which remains unchanged). Because of this, none of the tests need to call the iterableWithoutExtras or iteratorWithoutExtras |
| @@ -98,7 +99,7 @@ | |||
| * http://java.sun.com/j2se/1.4.2/docs/api/org/xml/sax/DTDHandler.html | |||
| */ | |||
|
|
|||
| public class CLDRFile implements Freezable<CLDRFile>, Iterable<String>, LocaleStringProvider { | |||
| public class CLDRFile implements Freezable<CLDRFile>, LocaleStringProvider { | |||
There was a problem hiding this comment.
Major change: CLDRFile no longer implements Iterable. Instead, a method must be called to get an iterable/iterator
| @@ -1462,22 +1463,70 @@ public static Set<String> getMatchingXMLFiles(File sourceDirs[], Matcher m) { | |||
| return s; | |||
| } | |||
|
|
|||
| @Override | |||
| public Iterator<String> iterator() { | |||
| private final boolean DEFAULT_ITERATION_INCLUDES_EXTRAS = true; | |||
There was a problem hiding this comment.
this boolean determines whether default iterator/iterable includes extra paths
|
|
||
| public Iterator<String> iteratorDefault() { | ||
| if (DEFAULT_ITERATION_INCLUDES_EXTRAS) { | ||
| return Iterators.filter(fullIterable().iterator(), p -> getStringValue(p) != null); |
There was a problem hiding this comment.
fullIterable is unchanged. If DEFAULT_ITERATION_INCLUDES_EXTRAS is true, iteratorDefault is the same as fullIterable except that it skips paths with null values
| return dataSource.iterator(); | ||
| } | ||
|
|
||
| public synchronized Iterator<String> iterator(String prefix) { | ||
| private synchronized Iterator<String> iteratorWithoutExtras(String prefix) { | ||
| return dataSource.iterator(prefix); |
There was a problem hiding this comment.
The original iterator methods are renamed to iteratorWithoutExtras and they are now private and rarely used if DEFAULT_ITERATION_INCLUDES_EXTRAS is true
| @@ -3081,7 +3146,7 @@ public String getWinningPath(String path) { | |||
| public Collection<String> getExtraPaths() { | |||
| Set<String> toAddTo = new HashSet<>(); | |||
| toAddTo.addAll(getRawExtraPaths()); | |||
| for (String path : this) { | |||
| for (String path : this.iterableWithoutExtras()) { | |||
There was a problem hiding this comment.
iterableWithoutExtras is required here to prevent infinite recursion, since the caller is getExtraPaths
| @@ -25,7 +25,7 @@ private CharacterFallbacks() { | |||
| DtdData.getInstance(DtdType.supplementalData).getDtdComparator(null); | |||
|
|
|||
| for (Iterator<String> it = | |||
| characterFallbacks.iterator("//supplementalData/characters/", comp); | |||
| characterFallbacks.iteratorDefault("//supplementalData/characters/", comp); | |||
| it.hasNext(); ) { | |||
| String path = it.next(); | |||
There was a problem hiding this comment.
All the callers of iterator outside of CLDRFile have become iteratorDefault. Note, BTW, that this loop, and many like it, should be simplified by calling iterableDefault instead of iteratorDefault. I've postponed that simplification, pending decision on the overall approach. Also, including the word "default" in the method names might be temporary; for now, it helps to indicate which code is affected by changing the default behavior from excluding to including extra paths
btangmu
requested review from
macchiati and
srl295
and removed request for
macchiati
|
I'm requesting review even though this is still a DRAFT PR. Question is how to proceed, not necessarily to merge this as-is. |
-Instead of Iterable interface, use new methods iterableDefault, iterableWithoutExtras
-Private final boolean CLDRFile.DEFAULT_ITERATION_INCLUDES_EXTRAS determines whether iteratorDefault and iterableDefault skip extras; either way they skip paths with null values
-Replace original iterator methods (zero or more parameters) with iteratorDefault or iteratorWithoutExtras
-Iterable and iterator are distinct, as in iterableWithoutExtras versus iteratorWithoutExtras
-Call iteratorWithoutExtras/iterableWithoutExtras directly where tests would otherwise fail with DEFAULT_ITERATION_INCLUDES_EXTRAS true
-Make methods private if possible without failing tests; surprisingly, all the WithoutExtras methods can be private
CLDR-18217
ALLOW_MANY_COMMITS=true