Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLDR-17014 Illustrate/fix extra-path bug #4256

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

btangmu
Copy link
Member

@btangmu btangmu commented Dec 24, 2024

-Writing a CLDRFile to XML failed for extra paths such as this metazone path as shown by the new test in TestSTFactory.xml

-Using cldrFile.fullIterable.forEach instead of xmlSource.forEach in CldrXmlWriter fixes the bug

CLDR-17014

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-Writing a CLDRFile to XML appears to fail for extra paths such as this metazone path
@btangmu
Copy link
Member Author

btangmu commented Dec 24, 2024

The same test failure occurs here that I saw locally:

Error: TestSTFactory.TestVettingDataDriven:469->runDataDrivenTest:492 93f Expected from XML aksalA at mul://ldml/dates/timeZoneNames/metazone[@type="Alaska"]/long/generic got null

The failure occurs in this code in DataDrivenSTTestHandler.java:

        // Verify from XML
        File outFile = new File(targDir, locale.getBaseName() + ".xml");
        if (outFile.exists()) outFile.delete();
        PrintWriter pw;
        pw = FileUtilities.openUTF8Writer(targDir.getAbsolutePath(), locale.getBaseName() + ".xml");
        cf.write(pw, TestSTFactory.noDtdPlease);
        pw.close();

        // System.out.println("Read back..");
        CLDRFile readBack = null;
        readBack = CLDRFile.loadFromFile(outFile, locale.getBaseName(), DraftStatus.unconfirmed);
        String reRead = readBack.getStringValue(xpath);
        String fullXpathBack = readBack.getFullXPath(xpath);
        Status xpathStatusBack;
        if (fullXpathBack == null || itMoved) {
            xpathStatusBack = Status.missing;
        } else {
            XPathParts xpp2 = XPathParts.getFrozenInstance(fullXpathBack);
            String statusFromXpathBack = xpp2.getAttributeValue(-1, "draft");

            if (statusFromXpathBack == null) {
                statusFromXpathBack = "approved"; // no draft =
                // approved
            }
            xpathStatusBack = Status.fromString(statusFromXpathBack);
        }

        if (value == null && reRead != null) {
            fail(
                    pathCount
                            + "e Expected null value from XML at "
                            + locale
                            + ":"
                            + xpath
                            + " got "
                            + reRead);
        } else if (value != null && !value.equals(reRead)) {
            fail(
                    pathCount
                            + "f Expected from XML "
                            + value
                            + " at "
                            + locale
                            + ":"
                            + xpath
                            + " got "
                            + reRead);

The error message includes "f Expected from XML ...".

The failure happens where it calls cf.write(pw, TestSTFactory.noDtdPlease) which invokes:

    public CldrXmlWriter(CLDRFile cldrFile, PrintWriter pw, Map<String, ?> options) {
        this.options = options;
        this.pw = pw;
        this.cldrFile = cldrFile;

        xmlSource = cldrFile.dataSource;
        orderedSet = new TreeSet<>(cldrFile.getComparator());
        xmlSource.forEach(orderedSet::add);

I think orderedSet is the set of paths to be written, and it does not include extra paths since it only derives from cldrFile.dataSource, skipping cldrFile.extraPaths. A solution might involve calling cldrFile.fullIterable instead.

Notice this old comment in CLDRFile:

XMLSource dataSource; // TODO(jchye): make private

In general it may be problematic that dataSource isn't private, and also that CLDRFile implements Iterable<String> but this method of iteration also doesn't include extra paths, for which cldrFile.fullIterable needs to be called explicitly. It might be better if the distinction between extra paths and regular paths were more encapsulated, and most code would get the full set of paths by default.

-Use cldrFile.fullIterable.forEach instead of xmlSource.forEach in CldrXmlWriter
@btangmu
Copy link
Member Author

btangmu commented Jan 9, 2025

The second commit to this PR fixes the test failure introduced by the first commit

@btangmu btangmu changed the title CLDR-17014 Illustrate extra-path bug with TestSTFactory.xml CLDR-17014 Illustrate/fix extra-path bug Jan 9, 2025
@btangmu
Copy link
Member Author

btangmu commented Jan 9, 2025

Testing in ST, without the changes in this PR, suggests that if I vote (as Admin) for a value for an extra path that had no value, and then I generate VXML, that path fails to be written. However, if I restart ST between voting and generating VXML, the path is written. Seemingly loadVoteValues (which calls internalSetVoteForValue, which calls makeSureInPathsForFile) in STFactory.java accounts for loading the votes when a locale is first opened after starting ST. It's maybe not surprising then that the bug hasn't been evident before when generating VXML.

@btangmu
Copy link
Member Author

btangmu commented Jan 10, 2025

@macchiati shall I switch the ticket from Design to Accepted, and merge this? Seems like the design -- to handle language paths similarly to timezone paths, as extra paths -- has turned out to be viable, this bug fix being the last hang-up I'm aware of. The other PR, with the lion's share of changes, could then be merged after a little more testing: #4254

@btangmu btangmu marked this pull request as ready for review January 10, 2025 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants