CLDR-18217 Add CLDRFile Iterable interface option to include extra paths by default #4371
Conversation
…fault -Private boolean CLDRFile.DEFAULT_ITERATION_INCLUDES_EXTRAS determines whether to skip extras; either way they skip paths with null values -Rename old iterator 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
|
I'm not sure whether this should go into v47 or wait for v48. Another option would be to make DEFAULT_ITERATION_INCLUDES_EXTRAS false for v47 (preserving current behavior), and change it to true for v48. There are risks either way. Changing a lot of paths to extra paths (without code-fallback values) has increased the risk that some unknown bugs exist due to skipping extra paths. Including extra paths in iteration could also introduce bugs. Either way, the bugs in question aren't covered yet by unit tests, and they could be tools that are run infrequently. |
|
Let's not change behavior from v46.1 this late in the game. |
Understood. What do you think about making DEFAULT_ITERATION_INCLUDES_EXTRAS false, and merging it that way? |
|
Only if we verify that there are no differences in resolved values.
…On Wed, Feb 12, 2025 at 12:03 PM Tom Bishop ***@***.***> wrote:
Let's not change behavior from v46.1 this late in the game.
Understood. What do you think about making
DEFAULT_ITERATION_INCLUDES_EXTRAS false, and merging it that way?
—
Reply to this email directly, view it on GitHub
<#4371 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJLEMEBUA4DHZ6RQVVBOQL2POSJZAVCNFSM6AAAAABXAK24RSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJUG4ZDMNRUGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
| private Iterator<String> extraPaths; | ||
|
|
||
| FullIterable(CLDRFile file) { | ||
| this.file = file; | ||
| this.fileIterator = file.iterator(); | ||
| this.fileIterator = file.dataSource.iterator(); // file.iteratorWithoutExtras(); |
There was a problem hiding this comment.
| this.fileIterator = file.dataSource.iterator(); // file.iteratorWithoutExtras(); | |
| this.fileIterator = file.iteratorWithoutExtras(); |
would it be better to use the function for documentation?
There was a problem hiding this comment.
Good idea. And I'll rename fileIterator to iteratorWithoutExtras to make it clear and consistent.
There was a problem hiding this comment.
I've done that, in the 2nd commit
…rectly -Also for clarity, rename FullIterable.fileIterator to FullIterable.iteratorWithoutExtras
Yes, that's wise. Locally, I will generate VXML, and also run GenerateProductionData and confirm no differences... |
-Keeping DEFAULT_ITERATION_INCLUDES_EXTRAS false for now seems safest if we merge this for v47
|
The 3rd commit makes DEFAULT_ITERATION_INCLUDES_EXTRAS false |
|
Locally, with these changes to CLDRFile (including DEFAULT_ITERATION_INCLUDES_EXTRAS false), I generated VXML by logging in as Admin and choosing Generate VXML from the main menu. Then I copied the VXML to common/main and ran GenerateProductionData. Then I repeated those steps on the main branch, with a different destination directory. Comparing the destination directories, both for VXML and for production, resulted in zero differences. |
btangmu
changed the title
|
Thanks! |
-Private boolean CLDRFile.DEFAULT_ITERATION_INCLUDES_EXTRAS determines whether to skip extras; either way they skip paths with null values
-Rename old iterator 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
CLDR-18217
ALLOW_MANY_COMMITS=true