CLDR-18217 Refactor by removing Iterable interface from CLDRFile #4280
Conversation
-Instead of Iterable interface, use new methods iterableDefault, iterableWithoutExtras -Private boolean CLDRFile.DEFAULT_ITERABLE_INCLUDES_EXTRAS determines whether iterableDefault calls fullIterable or iterableWithoutExtras -Call iterableWithoutExtras directly where tests would otherwise fail with DEFAULT_ITERABLE_INCLUDES_EXTRAS true -Rename original iterator method to iteratorWithoutExtras -Iterable and iterator are distinct: iterableWithoutExtras references iteratorWithoutExtras -There is not yet any iteratorDefault (only iterableDefault)
|
This draft PR is intended for investigation, not (at least not yet) for merging. Note that it has DEFAULT_ITERABLE_INCLUDES_EXTRAS true. If DEFAULT_ITERABLE_INCLUDES_EXTRAS were false it would merely be a refactoring of the current code, and the tests would pass trivially (assuming the refactoring is only that). With DEFAULT_ITERABLE_INCLUDES_EXTRAS, it was necessary to change some iterableDefault into iterableWithoutExtras in order to make the tests pass. It's an open question, when extra paths should be included when looping through the paths in a CLDRFile. See https://unicode-org.atlassian.net/browse/CLDR-17014 for motivation for believing the default should be that extra paths are included. The first merged PR for that ticket fixed a pre-existing bug involving metazone paths (which were already implemented using extra paths). There are probably more such bugs, not yet discovered, which could be fixed by making DEFAULT_ITERABLE_INCLUDES_EXTRAS true. |
|
By the way, the versions of |
| this.title = t; | ||
|
|
||
| for (String s : f) { | ||
| for (String s : f.iterableDefault()) { |
There was a problem hiding this comment.
This is the typical situation. Since CLDRFile no longer implements Iterable, it's necessary to call a method on it to get an iterable.
| CLDRFile mt = fac.make(locale, false); | ||
| BallotBox<User> box = fac.ballotBoxForLocale(locale); | ||
| mt.iterator(); | ||
| mt.iteratorWithoutExtras(); |
There was a problem hiding this comment.
This is "iterator" not "iterable"; the method has merely been renamed.
| CLDRFile input = factory.make(ORIGINAL_LOCALE, false); | ||
| XMLSource outputSource = new SimpleXMLSource(outputLocale); | ||
| for (String xpath : input) { | ||
| for (String xpath : input.iterableWithoutExtras()) { |
There was a problem hiding this comment.
Here, there would be a test failure if it were iterableDefault; likewise everywhere that iterableWithoutExtras is called outside of CLDRFile
| return orderedSet.iterator(); | ||
| } | ||
|
|
||
| private final boolean DEFAULT_ITERABLE_INCLUDES_EXTRAS = true; |
There was a problem hiding this comment.
Here's the key private boolean, referenced only in the method iterableDefault just below.
| Set<String> toAddTo = new HashSet<>(); | ||
| toAddTo.addAll(getRawExtraPaths()); | ||
| for (String path : this) { | ||
| for (String path : this.iterableWithoutExtras()) { |
There was a problem hiding this comment.
java.lang.StackOverflowError would occur here due to infinite loop if extras were included
| private Status checkAnnotations(CLDRFile cldrFile) { | ||
| Status status = Status.none; | ||
| for (String xpath : cldrFile) { | ||
| for (String xpath : cldrFile.iterableWithoutExtras()) { |
There was a problem hiding this comment.
test would fail if with extras
| private String getUncontainedPath(CLDRFile subset, CLDRFile superset) { | ||
| int debugCount = 0; | ||
| for (String xpath : subset) { | ||
| for (String xpath : subset.iterableWithoutExtras()) { |
There was a problem hiding this comment.
test would fail if with extras
| badPlusSample.clear(); | ||
| CLDRFile file = factory.make(locale, false); | ||
| for (String path : file) { | ||
| for (String path : file.iterableWithoutExtras()) { |
There was a problem hiding this comment.
test would fail if with extras
| Boolean.class); | ||
|
|
||
| for (String path : cldrFile) { | ||
| for (String path : cldrFile.iterableWithoutExtras()) { |
There was a problem hiding this comment.
test would fail if with extras
| // we get all the root paths with count | ||
| addPluralCounts(toAddTo, pluralCounts, pluralCountsRaw, this); | ||
| addPluralCounts( | ||
| toAddTo, pluralCounts, pluralCountsRaw, this.iterableWithoutExtras()); |
There was a problem hiding this comment.
StackOverflowError again if with extras
1 participant
-Instead of Iterable interface, use new methods iterableDefault, iterableWithoutExtras
-Private boolean CLDRFile.DEFAULT_ITERABLE_INCLUDES_EXTRAS determines whether iterableDefault calls fullIterable or iterableWithoutExtras
-Call iterableWithoutExtras directly where tests would otherwise fail with DEFAULT_ITERABLE_INCLUDES_EXTRAS true
-Rename original iterator method to iteratorWithoutExtras
-Iterable and iterator are distinct: iterableWithoutExtras references iteratorWithoutExtras
-There is not yet any iteratorDefault (only iterableDefault)
CLDR-18217
ALLOW_MANY_COMMITS=true