bump JSP to Unicode 14 #93

srl295 · 2021-07-12T19:19:50Z

Fixes: #88

see diary here: https://gist.github.com/srl295/a788b4de315cd919bb5f0f4e3b2f69c7 (search for 'SRL')

also:
Fixes #103

srl295 · 2021-07-15T20:23:54Z

blocked by and based on. #44

Step 1a text files For #88 - GenerateSubtagNames - Extra{Property,PropertyValue}Aliases.txt - PropertyAliases.txt from 14.0.0 - copied props with CopyPropsToUnicodeJsp

- leave some space between ICU and collections such as Hans - replace some synchornized and static init with Bill Pugh singletons for #88

- updated docs For: #88

For: #88

macchiati · 2021-07-20T21:40:03Z

UnicodeJsps/src/main/java/org/unicode/jsp/CachedProps.java

@@ -44,7 +44,7 @@
    final BiMultimap<String,String> nameToAliases = new BiMultimap<String,String>(null,null);
    final Map<String,BiMultimap<String,String>> nameToValueToAliases = new LinkedHashMap();

-    static CachedProps CACHED_PROPS = getInstance(VersionInfo.getInstance(12));


I'm surprised at this; if based on master it would be replacing 13 by 14.

Is this a bug? I hadn't changed this value. Should it be calculated ?

Lemme check.

Yes, that should be the version of the beta props. I think it is built that way so that it doesn't pull in the BIN properties if BETA is off. For now, let's just leave it at 14, but file an issue.

macchiati · 2021-07-20T21:44:29Z

unicodetools/org/unicode/text/utility/Settings.java

+        	 * @param version
+        	 * @return
+        	 */
+        	public String versionToString(VersionInfo version) {


There is a method on VersionInfo that produces a string with specified fields. Should be used instead of string building. Could be fixed later if you want.

I saw that but it is ICU @Internal and has its own behavior.

We use it in a bunch of places, and frankly there isn't a good reason for it not to be public. Mark

But this isn't a blocker at all!!

macchiati · 2021-07-20T23:03:43Z

We use it in a bunch of places, and frankly there isn't a good reason for it not to be public. Mark

…

On Tue, Jul 20, 2021 at 3:14 PM Steven R. Loomis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In unicodetools/org/unicode/text/utility/Settings.java <#93 (comment)> : > + */ + public Path asPath(VersionInfo forVersion) { + String versionString = versionToString(forVersion); + if (this == ucd) { + // For some reason, these have -Update + return asPath().resolve(versionString + "-Update"); + } else { + return asPath().resolve(versionString); + } + } + /** + * Map a version number to a string. + * @param version + * @return + */ + public String versionToString(VersionInfo version) { I saw that but it is ICU @internal and has its own behavior. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#93 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMCIZMTDZQUUEBHGMKDTYXYLLANCNFSM5AHOVP2A> .

markusicu · 2021-07-20T21:16:15Z

UnicodeJsps/src/main/java/org/unicode/jsp/CachedProps.java

@@ -44,7 +44,7 @@
    final BiMultimap<String,String> nameToAliases = new BiMultimap<String,String>(null,null);
    final Map<String,BiMultimap<String,String>> nameToValueToAliases = new LinkedHashMap();

-    static CachedProps CACHED_PROPS = getInstance(VersionInfo.getInstance(12));
+    static CachedProps CACHED_PROPS = getInstance(VersionInfo.getInstance(14));


Should this be driven by the version string in class Settings?

It's a bit more complicated than that because of the interplay with the beta flag. I suggested that we go with 14 for now, and file an issue. We don't want to wait on the fuller solution.

markusicu · 2021-07-20T21:17:33Z

UnicodeJsps/src/main/java/org/unicode/jsp/ScriptTester.java

@@ -9,6 +9,10 @@
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.Map.Entry;


I know that Eclipse is dumb about this, but it would be much better to only import Map, and use Map.Entry in call sites.

markusicu · 2021-07-20T21:20:45Z

UnicodeJsps/src/main/java/org/unicode/jsp/ScriptTester.java

+      // Verify we didn't run over
+      if (newCode >= LIMIT) {


Do we need a hard limit?

if we have more than 'extrascripts' scripts, yes

markusicu · 2021-07-20T21:42:49Z

UnicodeJsps/src/main/java/org/unicode/jsp/ScriptTester.java

+    // Scripts without stable numbers
+    {"Hant", "Han Traditional"}, {"Hans", "Han Simplified"},
+  };


Do we need these? ICU has UScript.SIMPLIFIED_HAN and UScript.TRADITIONAL_HAN.

I'm not sure.

markusicu · 2021-07-20T23:50:47Z

UnicodeJsps/src/main/java/org/unicode/jsp/ScriptTester.java

-  LIMIT = UScript.CODE_LIMIT; // HANS + 1;
-
-  private static String[][] EXTENDED_NAME = {{"Hant", "Han Traditional"}, {"Hans", "Han Simplified"}};
+  LIMIT = UScript.CODE_LIMIT + EXTRA_COUNT; // HANS + 1;


Please remove the comments about HANT/HANS -- since we have real UScript constants for them.

markusicu · 2021-07-20T23:56:37Z

docs/unicodejsps/index.md

@@ -1,4 +1,4 @@
-# Building UnicodeJsp
+git # Building UnicodeJsp


docs/unicodejsps/index.md

markusicu · 2021-07-20T23:58:57Z

unicodetools/org/unicode/propstest/CopyPropsToUnicodeJsp.java

-        String fromDir = Settings.Output.BIN_DIR + latest.getUcdVersion() + "/";
+        VersionInfo ucdVersion = latest.getUcdVersion();
+        System.out.println("Copying Props for " + ucdVersion + " into  JSP");
+		String fromDir = Settings.Output.BIN_DIR + ucdVersion + "/";


no tabs please

markusicu · 2021-07-20T23:59:28Z

unicodetools/org/unicode/propstest/ListProps.java

@@ -27,7 +28,8 @@

 public class ListProps {

-    private static final UcdProperty DEBUG_LIST_VALUES = null ; // UcdProperty.Confusable_MA;
+    private static final String BIN_PROPS = Settings.Output.BIN_DIR;
+	private static final UcdProperty DEBUG_LIST_VALUES = null ; // UcdProperty.Confusable_MA;


markusicu · 2021-07-21T00:06:37Z

unicodetools/org/unicode/text/utility/Settings.java

+	    		if (this != emoji) {
+	    			// 13.1, 14.0
+	    			sb.append(".")
+	    			  .append(version.getMicro());


getMicro() --> getMilli()

or indeed better call internal-for-CLDR-etc getVersionString()

markusicu · 2021-07-21T00:07:15Z

unicodetools/org/unicode/text/utility/Settings.java

        public static final String UCD_DIR = DATA_DIR + "ucd/";
        // TODO: IDN_DIR is used, but there is no .../data/IDN/ folder. Should this be .../data/idna/ ?
        public static final String IDN_DIR = DATA_DIR + "IDN/";
        // TODO: DICT_DIR is used, but there is no .../data/dict/ folder. ??
        public static final String DICT_DIR = DATA_DIR + "dict/";
+
+        public enum DataDir {
+        	security,


enum constants should be uppercase

markusicu · 2021-07-21T00:08:12Z

unicodetools/org/unicode/text/utility/Settings.java

@@ -70,11 +76,58 @@ private static final String getRequiredPathAndFix(String key) {
        public static final String UNICODETOOLS_DIR = UNICODETOOLS_REPO_DIR + "unicodetools/";
        public static final String UNICODEJSPS_DIR = UNICODETOOLS_REPO_DIR + "UnicodeJsps/";
        public static final String DATA_DIR = UNICODETOOLS_DIR + "data/";
+        public static final Path DATA_FILE = Paths.get(DATA_DIR);


markusicu · 2021-07-21T00:11:46Z

unicodetools/org/unicode/tools/UpdateJspFiles.java

+    }
+
+	public static void main(String args[]) throws IOException {
+        generateSubtagNames(); // this takes a sec, so run it first


indent 4 please

unicodetools/org/unicode/tools/UpdateJspFiles.java

- typo/tab fixes - add a unit test for Settings.UnicodeTools.DataDir - Apply other suggestions from code review Co-authored-by: Markus Scherer <markus.icu@gmail.com>

srl295 · 2021-07-21T17:08:10Z

OK, please take a look

macchiati

Go for it!

srl295 mentioned this pull request Jul 12, 2021

Issue when script codes are ahead of ICU #94

Open

srl295 force-pushed the srl295/issue88-jsp-14 branch from 7477414 to 0602af8 Compare July 14, 2021 15:04

srl295 added 3 commits July 20, 2021 11:15

jsp: to Beta 14.0.0 data (cut 1)

816672f

Step 1a text files For #88 - GenerateSubtagNames - Extra{Property,PropertyValue}Aliases.txt - PropertyAliases.txt from 14.0.0 - copied props with CopyPropsToUnicodeJsp

use beta

ebe22c7

make up scriptCodes for unrecognized scripts

a3cf705

- leave some space between ICU and collections such as Hans - replace some synchornized and static init with Bill Pugh singletons for #88

srl295 force-pushed the srl295/issue88-jsp-14 branch from 0602af8 to a3cf705 Compare July 20, 2021 16:16

srl295 added 3 commits July 20, 2021 14:19

mkdir to fix #103

9264c9a

Add UpdateJspFiles to update all JSP files.

c11afd2

- updated docs For: #88

re-run UpdateJspFiles for U14.0.0

3c5e85b

For: #88

srl295 marked this pull request as ready for review July 20, 2021 19:37

srl295 requested review from macchiati and markusicu July 20, 2021 19:37

srl295 mentioned this pull request Jul 20, 2021

lots of TODOs in UnicodeJsp update instructions #104

Open

srl295 self-assigned this Jul 20, 2021

different fix for #103 changes data

c9461c5

srl295 force-pushed the srl295/issue88-jsp-14 branch from 73f1434 to c9461c5 Compare July 20, 2021 21:11

macchiati previously approved these changes Jul 20, 2021

View reviewed changes

markusicu reviewed Jul 20, 2021

View reviewed changes

markusicu reviewed Jul 21, 2021

View reviewed changes

srl295 mentioned this pull request Jul 21, 2021

revisit cached_props #106

Open

srl295 dismissed macchiati’s stale review via 7e5bf75 July 21, 2021 16:57

srl295 force-pushed the srl295/issue88-jsp-14 branch from 7e5bf75 to 01eed8b Compare July 21, 2021 17:01

Updates per review comments

933d2b8

- typo/tab fixes - add a unit test for Settings.UnicodeTools.DataDir - Apply other suggestions from code review Co-authored-by: Markus Scherer <markus.icu@gmail.com>

srl295 force-pushed the srl295/issue88-jsp-14 branch from 8ffe009 to 933d2b8 Compare July 21, 2021 17:05

srl295 requested review from markusicu and macchiati July 21, 2021 17:06

macchiati approved these changes Jul 21, 2021

View reviewed changes

srl295 merged commit 122b9b9 into main Jul 21, 2021

srl295 deleted the srl295/issue88-jsp-14 branch July 21, 2021 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bump JSP to Unicode 14 #93

bump JSP to Unicode 14 #93

srl295 commented Jul 12, 2021 •

edited

Loading

srl295 commented Jul 15, 2021

macchiati Jul 20, 2021

srl295 Jul 20, 2021

macchiati Jul 20, 2021

macchiati Jul 20, 2021

macchiati Jul 20, 2021

srl295 Jul 20, 2021

macchiati Jul 21, 2021

macchiati commented Jul 20, 2021 via email

markusicu Jul 20, 2021

macchiati Jul 20, 2021

markusicu Jul 20, 2021

markusicu Jul 20, 2021

srl295 Jul 21, 2021

markusicu Jul 20, 2021

srl295 Jul 21, 2021

markusicu Jul 20, 2021

markusicu Jul 20, 2021

srl295 Jul 21, 2021

markusicu Jul 20, 2021

markusicu Jul 20, 2021

markusicu Jul 21, 2021

markusicu Jul 21, 2021

markusicu Jul 21, 2021

markusicu Jul 21, 2021

srl295 commented Jul 21, 2021

macchiati left a comment

		@@ -1,4 +1,4 @@
		# Building UnicodeJsp
		git # Building UnicodeJsp

bump JSP to Unicode 14 #93

bump JSP to Unicode 14 #93

Conversation

srl295 commented Jul 12, 2021 • edited Loading

srl295 commented Jul 15, 2021

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

macchiati commented Jul 20, 2021 via email

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 commented Jul 21, 2021

macchiati left a comment

Choose a reason for hiding this comment

srl295 commented Jul 12, 2021 •

edited

Loading