CLDR-17014 No code fallbacks for language paths #4254
Conversation
-Use the extra-paths mechanism instead of code-fallback for language paths
Yes, that sounds good, in fact I was thinking to follow this up with a refactoring PR, to move the extra-path code (not just for languages, but for all extra paths) out of CLDRFile into its own top-level class. It's still not clear to me what part, if any, of extra-path code is locale-specific; the current methods aren't static. It does seem wasteful to re-run code for each locale (each CLDRFile) if it's not locale-specific. Stepping back, though, does the general solution seem right, to use extra paths instead of code fallback for language paths? There are some test failures, like this, that I still need to study: |
|
Everything in code-fallback is locale independent and immutable. So that's why you can move it all to the new class, and fetch it with no local parameter. We might be able to just call that when building the root cldrfile. The other stuff in get extra paths is locale dependent. That could also be moved to the new class, but needs a locale parameter when fetched. |
Yes, with the changes I mention. |
-Change es-419 to es (419) in localeDisplayName.txt so TestLocaleDisplay passes -Add language path in testExtraPaths so it passes -Remove XMLSource.CODE_FALLBACK_ID, GERMAN, from testGetPaths so it passes
|
The second commit fixes some but not all test failures. The remaining test failures are TestCoverageCompleteness and testLSR, both in TestCoverageLevel. These need more investigation. After the tests are all passing, I'll address refactoring/optimizing with a new class ExtraPaths. |
-Create the set of language extra paths only once; related refactoring -Fix another part of testGetPaths so it passes, similar to previous commit -Comments
| private void getLanguageExtraPaths(Set<String> toAddTo) { | ||
| Set<String> codes = | ||
| StandardCodes.make().getSurveyToolDisplayCodes(NameType.LANGUAGE.getNameName()); | ||
| codes.remove(XMLSource.ROOT_ID); |
There was a problem hiding this comment.
this is wrong, my mistake -- it actually modifies the set in StandardCodes! Need to make a copy of the set. We might want to prevent this kind of bug by making getSurveyToolDisplayCodes return an unmodifiable set, maybe using Set.copyOf.
We currently have this, which doesn't work: CLDRLanguageCodes = CldrUtility.protectCollection(CLDRLanguageCodes);
There was a problem hiding this comment.
Fixed by my last commit -- we still should look into Set.copyOf as an alternative to protectCollection
There was a problem hiding this comment.
Important: In general, use ImmutableSet.copyOf instead of Set.copyOf. The latter changes the order in the set, which may be important (TreeSet or LinkedHashSets get messed up, and it doesn't hurt for HashSet).
There was a problem hiding this comment.
Also, we need to fix CldrUtility.protectCollection(CLDRLanguageCodes); — if it does't work then a lot of other things could go wrong.
-Do not modify SupplementalDataInfo.CLDRLanguageCodes, returned by sc.getGoodAvailableCodes!
| @@ -1073,7 +1075,7 @@ public void testLSR() { | |||
|
|
|||
| // Get root LSR codes | |||
|
|
|||
| for (String path : root) { | |||
| for (String path : root.fullIterable()) { | |||
There was a problem hiding this comment.
without fullIterable, the loop skipped the extra paths
this may be a concern in general: how often do we loop through a CLDRFile the simple way, which skips extra paths, and what are the consequences and rationale for that?
|
The test failures all seem fixed now except for TestVettingDataDriven |
-Use the extra-paths mechanism instead of code-fallback for language paths
CLDR-17014
ALLOW_MANY_COMMITS=true