CLDR-15830 Extensive refactoring of NameGetter; new enum NameType #4246

btangmu · 2024-12-15T23:02:35Z

-New enum NameType replaces integers CLDRFile.LANGUAGE_NAME, etc.; avoid arithmetic based on such integers

-Encapsulate in NameType all dependencies on ordering of NameType.LANGUAGE, etc.

-Move related methods and NameTable from CLDRFile to NameType

-Rename NameGetter.getNameFromBCP47 to getNameFromIdentifier, etc., since identifiers may not be only BCP47; change some comments accordingly

-Avoid boolean parameter onlyConstructCompound; use enum NameGetter.NameOpt instead, values COMPOUND_ONLY and DEFAULT

-Fix all 3 TODO comments added in previous PR 4228

-Fix typo: rename getZoneToCounty to getZoneToCountry (country, not county)

-Remove dead code, including ShowLanguages.printMissing, ShowLanguages.print, TestCoverageLevel.TypeName

-Remove some commented-out code

This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-Fix all 3 TODO comments added in previous PR

…ME, etc. -Replace many but not all int CLDRFile.LANGUAGE_NAME with Enum NameType.LANGUAGE, and so forth -New method getNameFromTypeEnumCode(NameType, ...) to replace getNameFromTypenumCode(int, ...) -New method NameType.getKeyPath to replace CLDRFile.getKey, work in progress -Remove CLDRFile.TZ_START and CLDRFile.TZ_LIMIT -New temporary methods NameType.fromCldrInt and NameType.toCldrInt, only to facilitate transition -New TODO comments in CLDRTest.java

…try, not county)

…ME, etc. -Fix TestDisplayNameCollisions to use NameType instead of int -New NameType.getNameTypeName, wraps CLDRFile.getNameTypeName for now -New NameType.fromPath like CLDRFile.getNameType -New NameType.NONE like CLDRFile.NO_NAME -Remove some commented-out code

-Fix ShowLanguages, TestScriptMetadata to use NameType instead of int

-Change int to NameType, avoid arithmetic

…etCode/getNameType -Also remove an unused private method getName from TimezoneFormatter

…omTypenumCode -Also move the NameTable-associated methods, and consolidate -Change more int to NameType -Make toCldrInt private -Remove dead code

-Encapsulate in NameType all dependencies on ordering of NameType.LANGUAGE, etc. -Remove dead code, including ShowLanguages.printMissing, ShowLanguages.print, TestCoverageLevel.TypeName

…instead of boolean -Rename getNameFromBCP47 to getNameFromIdentifier, since may not be only BCP47; change some comments accordingly -Avoid boolean parameter onlyConstructCompound; use enum NameGetter.NameOpt instead, values COMPOUND_ONLY and DEFAULT

btangmu · 2024-12-15T23:27:04Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/ShowLanguages.java

-                }
-            }
-            pw.println("</table></div>");
-        }


The above two methods were unused, and I deleted them rather than refactor them

btangmu · 2024-12-16T00:45:50Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CLDRTest.java

@@ -922,10 +925,13 @@ public void TestMinimalLocalization() throws IOException {
                                    new String[] {
                                        "sun", "mon", "tue", "wed", "thu", "fri", "sat"
                                    }));
-            for (int i = -7; i < 0; ++i) {
-                checkForItems(item, (i < -4 ? months : days), i, missing, failureCount, null);


This usage of the magic number "-7" to mean "CLDRFile.TZ_EXEMPLAR and getDateKey", and so forth, involved some tricky arithmetic. This PR reduces the trickiness, at the expense of making the parameters explicit instead of looping.

much clearer

Add this as a comment to the code -- otherwise it hard for us in the future to follow why this is.

A practice I liked, is to move magic numbers to the top of the file as constant integers so when you call on them you call on English-readable labels.

Fortunately my change here removes the magic numbers, so I don't think there's any need for a comment

for (int i = -7; i < 0; ++i) { is the old code being replaced

agreed, this is a review on the old code. no comment required.

btangmu · 2024-12-16T00:47:04Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/CLDRFile.java

-     * @return
-     */
-    public static int getNameType(String xpath) {
-        for (int i = 0; i < NameTable.length; ++i) {


this code in CLDRFile.java moved (in refactored form) to NameType.java

btangmu · 2024-12-16T00:47:34Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/StandardCodes.java

+        public NameType toNameType() {
+            switch (this) {
+                case language:
+                    return NameType.LANGUAGE;


This maps StandardCodes.CodeType to NameType. These enums are strangely overlapping, suggesting possible future integration

btangmu · 2024-12-16T00:47:55Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/StandardCodes.java

+        public NameType toNameType() {
+            switch (this) {
+                case region:
+                    return NameType.TERRITORY;


Similarly this maps StandardCodes.LstrType to NameType. I'm not sure how StandardCodes.LstrType and StandardCodes.CodeType are related; again, they seem to overlap

they definitely overlap

btangmu · 2024-12-16T00:48:22Z

tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCoverageLevel.java

@@ -368,48 +368,6 @@ static String isBigLanguage(String lang) {

    private static final boolean DEBUG = false;

-    static class TypeName implements Transform<String, String> {


This was unused

btangmu · 2024-12-16T00:52:22Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/XMLSource.java

@@ -1601,31 +1601,39 @@ public String getLocaleID() {
        static {
            StandardCodes sc = StandardCodes.make();
            Map<String, Set<String>> countries_zoneSet = sc.getCountryToZoneSet();
-            Map<String, String> zone_countries = sc.getZoneToCounty();
-
-            for (int typeNo = 0; typeNo <= CLDRFile.TZ_START; ++typeNo) {


Here was a loop depending on the numeric values of CLDRFile.LANGUAGE_NAME, etc. The new code lists the enum values explicitly. This static code block is crucial for code fallback, and while studying it for https://unicode-org.atlassian.net/browse/CLDR-17014 and also seeing how it involves getName, it appeared that refactoring would make that ticket easier to understand and solve

btangmu · 2024-12-16T00:53:38Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameType.java

+
+public enum NameType {
+    NONE,
+    LANGUAGE,


These are the replacements for CLDRFile.NO_NAME, CLDRFile.LANGUAGE_NAME, etc. (which were integers, NOT enum)

I really like putting this in an enum :D

However, I'm having a hard time understanding what NameType means -- can you add a longer comment about what it means? I wonder if there's a better enum name.

As I just wrote in another reply, "A path for a language name is associated with NameType.LANGUAGE; a path for a script name is associated with NameType.SCRIPT; etc."

There are different types of names, and this enum enumerates those types, so I think it's a pretty good name... You're right, though, adding a comment here could help

could be a future PR to add some brief enum docs (at the enum level)

btangmu · 2024-12-16T00:54:00Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameType.java

+    // The row with "key|type" has four strings, while the others have three.
+    private static final String[][] NameTable = {
+        {"//ldml/localeDisplayNames/languages/language[@type=\"", "\"]", "language"},
+        {"//ldml/localeDisplayNames/scripts/script[@type=\"", "\"]", "script"},


NameTable is moved from CLDRFile, otherwise unchanged. Further refactoring could be beneficial, to avoid the entanglement with integers INDEX_LANGUAGE, etc., but at least that entanglement is now encapsulated inside this class

yeah, actually something that works with XPathParts (thus cache) - an XPathCalculator (builder) or something could be a completely different utility. That's what this is, it knows how to make an XPath for certain purposes.

btangmu · 2024-12-16T01:11:16Z

tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestLocale.java

@@ -238,15 +239,15 @@ public void TestLocalePartsValidity() {
            if (!xpath.startsWith("//ldml/localeDisplayNames/")) {
                continue;
            }
-            switch (CLDRFile.getNameType(xpath)) {
-                case 0:


0, 1, and 2 were used here instead of CLDRFile.LOCALE_NAME, CLDRFile.SCRIPT_NAME, and CLDRFile.TERRITORY_NAME

srl295

looking good so far

srl295 · 2024-12-17T18:59:50Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CLDRTest.java

@@ -922,10 +925,13 @@ public void TestMinimalLocalization() throws IOException {
                                    new String[] {
                                        "sun", "mon", "tue", "wed", "thu", "fri", "sat"
                                    }));
-            for (int i = -7; i < 0; ++i) {
-                checkForItems(item, (i < -4 ? months : days), i, missing, failureCount, null);


much clearer

srl295 · 2024-12-17T19:01:27Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/ChartAnnotations.java

@@ -189,7 +189,7 @@ public void writeSubcharts(Anchors anchors) throws IOException {
        for (Entry<LanguageGroup, Set<R3<Integer, String, String>>> groupPairs :
                groupToNameAndCodeSorted.keyValuesSet()) {
            LanguageGroup group = groupPairs.getKey();
-            String ename = nameGetter.getNameFromBCP47Bool("en", true);
+            String ename = nameGetter.getNameFromIdentifierCompoundOnly("en");


these are a lot better.

srl295 · 2024-12-17T19:02:09Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/StandardCodes.java

+        public NameType toNameType() {
+            switch (this) {
+                case region:
+                    return NameType.TERRITORY;


they definitely overlap

conradarcturus · 2024-12-17T19:23:13Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CLDRTest.java

-                                + xpath
-                                + "\t"
-                                + xpath2);
+                String theName = nameType.getNameTypeName();


What is it the name of? There are too many variables like "noun2" -- and reading this code without having written it, I don't know what "theName" really is.

Join the party :-) Doing this refactoring, I've also been diving into a lot of unfamiliar and perplexing code. The same variable or parameter may be called a name, or a type, or a code, ... This method in particular is looping through all the paths, so depending on the path, theName could be the name of a language, a script, a territory, ... -- any of the values of the new NameType enum. A path for a language name is associated with NameType.LANGUAGE; a path for a script name is associated with NameType.SCRIPT; etc.

conradarcturus · 2024-12-17T19:26:04Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CLDRTest.java

@@ -922,10 +925,13 @@ public void TestMinimalLocalization() throws IOException {
                                    new String[] {
                                        "sun", "mon", "tue", "wed", "thu", "fri", "sat"
                                    }));
-            for (int i = -7; i < 0; ++i) {
-                checkForItems(item, (i < -4 ? months : days), i, missing, failureCount, null);


Add this as a comment to the code -- otherwise it hard for us in the future to follow why this is.

A practice I liked, is to move magic numbers to the top of the file as constant integers so when you call on them you call on English-readable labels.

conradarcturus · 2024-12-17T19:27:51Z

tools/cldr-code/src/main/java/org/unicode/cldr/test/CLDRTest.java

@@ -843,6 +843,12 @@ void getSupplementalData(
        }
    }

+    /** Settings for checkForItems behavior */
+    private enum KeyOpt {


Is this an optional key? Is this a specific key possibility? What is the key for? It would be great to make variable names clearer.

It's a required parameter for one function, checkForItems. There are also two wrapper versions of checkForItems with fewer parameters, in which it implicitly has the value KeyOpt.DEFAULT, and there's one wrapper function checkForItemsDate in which it implicitly has the value KeyOpt.DATE.

Its purpose is to control the behavior of the only place it's referenced:

String key = (keyOpt == KeyOpt.DATE) ? getDateKey(nameType, code) : nameType.getKeyPath(code);

I don't know why exactly that's required; this is old code that I'm refactoring. These are the only callers where it implicitly has KeyOpt.DATE:

checkForItemsDate(item, months, NameType.TZ_EXEMPLAR, missing, failureCount); checkForItemsDate(item, months, NameType.CURRENCY_SYMBOL, missing, failureCount); checkForItemsDate(item, months, NameType.CURRENCY, missing, failureCount); checkForItemsDate(item, days, NameType.VARIANT, missing, failureCount); checkForItemsDate(item, days, NameType.TERRITORY, missing, failureCount); checkForItemsDate(item, days, NameType.SCRIPT, missing, failureCount); checkForItemsDate(item, days, NameType.LANGUAGE, missing, failureCount);

It's testing something about months and days, for which evidently getDateKey is required.

Rather than take a lot of additional time, which might not have been justified, to figure out what this whole test accomplishes and why, I approached the refactoring as a formal problem: how can I revise this code so that it still accomplishes the same result, without depending on the magic values of the numbers -7, etc. The function has two options for how to get a key, and instead of making one of the parameters negative or positive to indicate the option, it now has a KeyOpt parameter with two possible values.

When there's time and motivation for further refactoring, a builder pattern might be employed to avoid the large number of parameters...

I agree; we can make further improvements in later PRs now that the bulk of this is done.

However, as a double check before merging, I would generate before and after charts, then diff to see that each generated files is identical (generate both on the same day, so there are not spurious diffs).

@conradarcturus it's to be an improvement of a boolean parameter like "useDate" or something, because then you call it with getDateKey(nameType, true) where true has no obvious meaning

generate before and after charts, then diff to see that each generated files is identical

I will do that before merging

conradarcturus · 2024-12-17T19:30:52Z

tools/cldr-code/src/main/java/org/unicode/cldr/util/NameType.java

+
+public enum NameType {
+    NONE,
+    LANGUAGE,


I really like putting this in an enum :D

However, I'm having a hard time understanding what NameType means -- can you add a longer comment about what it means? I wonder if there's a better enum name.

srl295

great cleanup.

macchiati · 2024-12-18T17:22:26Z

Thanks!

…

On Wed, Dec 18, 2024 at 5:55 PM Tom Bishop ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tools/cldr-code/src/main/java/org/unicode/cldr/test/CLDRTest.java <#4246 (comment)>: > @@ -843,6 +843,12 @@ void getSupplementalData( } } + /** Settings for checkForItems behavior */ + private enum KeyOpt { generate before and after charts, then diff to see that each generated files is identical I will do that before merging — Reply to this email directly, view it on GitHub <#4246 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMAJNXJOWK5SU73YVDD2GGSHRAVCNFSM6AAAAABTU7IP46VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMJSGM4DKOBTGI> . You are receiving this because your review was requested.Message ID: ***@***.***>

btangmu · 2024-12-19T20:18:27Z

As @macchiati advised, I ran GenerateAllCharts with branch main, moved ../cldr-staging/docs/charts/47 to another location, then ran GenerateAllCharts again with branch t15830_c. The resulting ../cldr-staging/docs/charts/47 matched the version from main, comparing with diff -r -- no differences.

Some errors were reported by GenerateAllCharts both times, all similar to this:

Err: on path //supplementalData/calendarData/calendar[@type="japanese"]/eras/era[@type="106"][@start="1184-4-16"], missing territories. Probably, this tool is out of date.

I don't know what to make of that but it was the same regardless of the changes in this PR so I'm assuming it's unrelated, and I'm going ahead with merging this PR. I'll follow up with an explanatory comment on the NameType enum as @conradarcturus advised.

srl295 · 2024-12-19T20:23:39Z

yeah, I think we talked about a ticket to reduce the manual error checking

btangmu added 11 commits December 14, 2024 18:59

CLDR-15830 Eliminate getNameFromTypestrCode

80bf2c4

-Fix all 3 TODO comments added in previous PR

CLDR-15830 Fix typo: rename getZoneToCounty to getZoneToCountry (coun…

a6d2be7

…try, not county)

CLDR-15830 More int to Enum

a797273

-Fix ShowLanguages, TestScriptMetadata to use NameType instead of int

CLDR-15830 Fix magic numbers in CLDRTest.checkForItems

ff1b824

-Change int to NameType, avoid arithmetic

CLDR-15830 Start migrating CLDRFile.getCode/getNameType to NameType.g…

e1d4e57

…etCode/getNameType -Also remove an unused private method getName from TimezoneFormatter

CLDR-15830 Change more CLDRFile methods to NameType methods

2e16740

CLDR-15830 Move NameTable from CLDRFile to NameType; remove getNameFr…

e79ee94

…omTypenumCode -Also move the NameTable-associated methods, and consolidate -Change more int to NameType -Make toCldrInt private -Remove dead code

CLDR-15830 Remove CLDRFile.LANGUAGE_NAME, etc.

5eb6924

-Encapsulate in NameType all dependencies on ordering of NameType.LANGUAGE, etc. -Remove dead code, including ShowLanguages.printMissing, ShowLanguages.print, TestCoverageLevel.TypeName

github-actions bot assigned btangmu Dec 15, 2024

btangmu commented Dec 15, 2024

View reviewed changes

CLDR-15830 Fix typo Indentifier should be Identifier

1e740dc

btangmu commented Dec 16, 2024

View reviewed changes

btangmu marked this pull request as ready for review December 16, 2024 00:56

btangmu requested review from srl295 and macchiati December 16, 2024 00:57

btangmu commented Dec 16, 2024

View reviewed changes

srl295 reviewed Dec 17, 2024

View reviewed changes

conradarcturus reviewed Dec 17, 2024

View reviewed changes

srl295 approved these changes Dec 18, 2024

View reviewed changes

srl295 reviewed Dec 18, 2024

View reviewed changes

btangmu merged commit 72b7e8a into unicode-org:main Dec 19, 2024
12 checks passed

btangmu deleted the t15830_c branch December 19, 2024 20:19

		@@ -368,48 +368,6 @@ static String isBigLanguage(String lang) {

		private static final boolean DEBUG = false;

		static class TypeName implements Transform<String, String> {

CLDR-15830 Extensive refactoring of NameGetter; new enum NameType #4246

CLDR-15830 Extensive refactoring of NameGetter; new enum NameType #4246

Conversation

btangmu commented Dec 15, 2024

Choose a reason for hiding this comment

btangmu Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 left a comment

Choose a reason for hiding this comment

macchiati commented Dec 18, 2024 via email

btangmu commented Dec 19, 2024

srl295 commented Dec 19, 2024

btangmu Dec 16, 2024 •

edited

Loading