Mavenize UnicodeTools (#64) #91

srl295 · 2021-07-05T22:00:48Z

update unicodetools/ to be built with Maven
move data.picker data and classes into source, all other data in data/ is left where it is.
restructure tests to be JUnit

Fixes #64

srl295 · 2021-07-12T20:39:45Z

snapshot of disabled tests. I think all of these were not-working under Eclipse.

unicodetools/src/test/java//org/unicode/draft/MessageFormatCheck.java:@Disabled("All Broken")
unicodetools/src/test/java//org/unicode/draft/TestSourceTarget.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/draft/TestCalendar.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/draft/TestCalendar.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/draft/TestCalendar.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/draft/TestCalendar.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestIdnaTest.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestIdnaTest.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestIdnaTest.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestIdnaTest.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestLocaleConstruction.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestSettings.java:     @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestLocaleCanonicalization.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestLocaleValidity.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/unittest/TestSegmenter.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestEmojiDataConsistency.java:    @Disabled("Broken? data load issue")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestEmojiDataConsistency.java:    @Disabled("Broken? data load issue")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestEmojiOrder.java:@Disabled("Broken? data load issue")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestCandidateData.java:@Disabled("broken?")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestCandidateData.java: @Disabled("broken?")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestProposalData.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestProposalData.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestProposalData.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestProposalData.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestEmojiData.java:@Disabled("data will not load")
unicodetools/src/test/java//org/unicode/tools/emoji/unittest/TestEmojiData.java:    @Disabled("Not working yet")
unicodetools/src/test/java//org/unicode/test/TestUnicodeMapParser.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/test/TestUnicodeMapParser.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/idna/TestTransform.java:    @Disabled("Broken - Requested array size exceeds VM limit in org.unicode.idna.FilteredUnicodeTransform.transform(FilteredUnicodeTransform.java:31)    ")
unicodetools/src/test/java//org/unicode/propstest/TestXUnicodeSet.java: @Disabled("Broken")
unicodetools/src/test/java//org/unicode/propstest/TestImmutableUnicodeMap.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/propstest/TestImmutableUnicodeMap.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/propstest/TestScriptInfo.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/propstest/TestInvariants.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/propstest/TestProperties.java:    @Disabled("Broken")
unicodetools/src/test/java//org/unicode/propstest/TestScriptMetadata.java:    @Disabled("Broken")

srl295 · 2021-08-16T20:23:05Z

merge to keep this up to date. Will squash it later into fewer commits.

srl295 · 2021-09-20T19:50:12Z

Lots of commits to enable review… some of the commits are bulk renames.

Ready, if tests pass…

srl295 · 2021-09-20T20:33:05Z

todo:

Update docs
push icu.version and cldr.version properties or even dependencymanagement into the parent pom ( from jsp: move to Maven #41 )
update icu.version and cldr.version

@markusicu windows-setup.md seems to be very svn-centric… is it still up to date and/or needed? At least the parts about building (ant, etc) could be removed.

srl295 · 2021-09-21T22:39:13Z

Added docs

markusicu · 2021-09-27T19:53:34Z

@markusicu windows-setup.md seems to be very svn-centric… is it still up to date and/or needed? At least the parts about building (ant, etc) could be removed.

I don't work on Windows. When I moved the docs into GitHub, I touched that page just lightly, leaving the rest for Laurentiu or whoever else is using Windows.

docs/index.md

pom.xml

unicodetools/pom.xml

srl295 · 2021-09-28T16:37:13Z

todo:

review comments (thanks!)
Squash the squashable.
update ALL commit metadata so each commit could standalone metadata wise if need be.
Merge it

Fixes: #64 X-PR-URL: #91

- bump surefire and other tests - at least make sure it can compile Fixes: #64 X-PR-URL: #91

- bump surefire and other tests - at least make sure it can compile - build all branches - keep name thesame because of github bug - wonderful settings fix - even more fun with settings - .. and credentials Fixes: #64 X-PR-URL: #91

Fixes: #64 X-PR-URL: #91

srl295 · 2021-09-28T22:36:49Z

@echeran I've restructured the commits, should be ready to go

I think i'd like to get this in and then start getting folks testing it

echeran · 2021-09-29T04:37:18Z

@echeran I've restructured the commits, should be ready to go

I think i'd like to get this in and then start getting folks testing it

Ok, sounds fine.

Kudos for the inclusion of command-line instructions -- this is important because if Maven instructions work at the command line, then pretty much any major IDE can easily import the project and behave similarly without us having a hard dependency on Eclipse. Other IDEs these days are much snappier for Java, in my experience.

I tried following along to see if I could follow along with the instructions (gh pr checkout 91 --repo unicode-org/unicodetools; mkdir -p ../Generated; mvn exec:java -Dexec.mainClass= ...) but I ended up having to spy on the Github Actions workflow and guess at which commands to cherry pick and how to modify them in order to get something to run....

If not in this PR, then soon after, we should try to spruce up the instructions so that even someone like me can follow -- something that takes no more critical thinking than "1) open pouch 2) place in toaster 3) eat" -- I'm happy to help, so feel free to file a followup issue and assign to me. That may or may not help others test out, too.

srl295 · 2021-09-29T19:03:39Z

pretty much any major IDE can easily import the project and behave similarly without us having a hard dependency on Eclipse.

Yes exactly this! I use vs code mostly.

open pouch, put in toaster

I think this mostly has to do with the settings situation. Should be follow up prs.

The GitHub actions only exercises one path I was able to make work, may not be the best for testing.

Will merge thanks 🎉

srl295 · 2021-09-29T19:04:18Z

Oh I did conflict with myself. Oops.

srl295 · 2021-09-29T21:46:45Z

@markusicu can i get a RSLGTM?

srl295 · 2021-09-29T21:47:03Z

@markusicu or may I promote @echeran to a comitter?

unicodetools/src/main/java/org/unicode/picker/CharData.java

markusicu · 2021-10-01T19:32:10Z

unicodetools/src/main/java/org/unicode/text/UCA/UCA.java

@@ -199,7 +199,7 @@ public void remove() {
        }
    }

-    private final class PrimaryIterable implements Iterable<UCA.Primary> {
+    public final class PrimaryIterable implements Iterable<UCA.Primary> {


why public if we didn't need that before?

I think this was to work around a JDK issue.

[ERROR] /Users/srl295/src/unicodetools/unicodetools/src/main/java/org/unicode/text/UCA/WriteCollationData.java:[1805,74] org.unicode.text.UCA.UCA.PrimaryIterable.iterator() is defined in an inaccessible class or interface

yes. getIgnorableAndRegularPrimaries() returns a PrimaryIterable, and WriteCollationData tries to iterate over it. I hate to ask but how did this work before? Maybe getIgnorableAndRegularPrimaries() should return the interface Iterable<UCA.Primary> if the class was meant to be kept private?

Well, it did work this summer when I generated UCA data files several times.
I wonder if a later JDK got stricter? (Since you are upgrading to 11 I think.)

If the call sites don't need to know the specific impl class, then returning Iterable<UCA.Primary> would be fine.

pom.xml

markusicu

mostly lgtm

Thanks for all of this work, and also moving the tests to JUnit!
(But now I have to learn a new flavor of JUnit...)

The message on the second commit does not seem to describe the few changes there. Worth fixing? Or squash with the third commit?

There are still some unresolved/unreplied comments/questions from Elango.

srl295 · 2021-10-01T19:59:06Z

mostly lgtm

Thanks for all of this work, and also moving the tests to JUnit! (But now I have to learn a new flavor of JUnit...)

👍

just to distinguish, you can use the prior (TestFmwk) style of asserts still if preferred.

The message on the second commit does not seem to describe the few changes there. Worth fixing? Or squash with the third commit?

I'll take a look.

There are still some unresolved/unreplied comments/questions from Elango.

Will also take a look

- tests: rename many test files - pom: bump surefire and other tests - workflow: at least make sure it can compile, build all branches, credentials Fixes: #64 X-PR-URL: #91

Fixes: #64 X-PR-URL: #91

- move additional test files around Fixes: #64 X-PR-URL: #91

- disable more tests that did not work previously - fix compile issues - use JDK11 to avoid build issue > unicodetools/unicodetools/src/test/java/org/unicode/propstest/RegexWordBreak.java:[133,44] incompatible types: inference variable T has incompatible equality constraints java.lang.Enum,capture#1 of ? extends java.lang.Enum - improve how CLDR dir is checked out - mkdir Generated/BIN etc when needed - try to generate unicode data on CI - create output dir first - fix Generated path Documentation: - bump CLDR/ICU deps into top level - update docs on how to use maven - add docs on updating CLDR/ICU versions - update workflows to use top level maven file - update docs on setup/updating - pom versions to 1.0.0 Fixes: #64 X-PR-URL: #91

- fix broken lines in CharData.java - cleanup maven poms Move more configuration into the top level pom.xml, add some comments Fixes: #64 X-PR-URL: #91

srl295 · 2021-10-01T20:19:26Z

@markusicu @echeran Ok -

reworded 2nd commit (squashed the two 'move tests around' commits together, one was a delta of the other)
merged today's 'review comment' changes into the last commit

srl295 · 2021-10-01T20:29:04Z

btw,

Thanks for all of this work

being on either side of the Review button on these mega-changes is never a walk in the park, so thank you. I'm optimistic that easier onboarding to this project will really benefit ucd-dev and thus Unicode.

srl295 · 2021-10-01T20:33:21Z

Also I don't have a "Phase II" planned, so I took out the "Phase I". I think I originally thought there might be some smaller steps, but they didn't make as much sense.

Followons will include the deduplicating of code by making UnicodeJsps depend on unicodetools, but that's for #66

Fixes #58 (note that #91 already corrects the Java part of this)

echeran

LGTM

markusicu

lgtm

markusicu · 2021-10-01T21:47:51Z

pom.xml

+					<configuration>
+						<systemPropertyVariables>
+							<!-- These are variables which are picked up by test runs -->
+							<UVERSION>13.0.0</UVERSION>


14? later PR?

srl295 self-assigned this Jul 5, 2021

srl295 closed this Jul 5, 2021

srl295 reopened this Jul 5, 2021

srl295 force-pushed the srl295/issue64 branch from 2940d60 to 61b57c6 Compare July 5, 2021 22:11

srl295 changed the title ~~#64 ⚠️ danger danger ⚠️~~ #64 ⚠️ danger danger ⚠️ (Mavenize unicodetools) Jul 12, 2021

srl295 force-pushed the srl295/issue64 branch 4 times, most recently from 656903a to 3a2f404 Compare July 13, 2021 15:23

srl295 changed the title ~~#64 ⚠️ danger danger ⚠️ (Mavenize unicodetools)~~ Mavenize UnicodeTools (#64) Phase I Jul 17, 2021

srl295 force-pushed the srl295/issue64 branch 2 times, most recently from 379e679 to 78f05c6 Compare August 16, 2021 20:39

srl295 force-pushed the srl295/issue64 branch from 78f05c6 to 796b2ae Compare September 20, 2021 19:49

srl295 marked this pull request as ready for review September 20, 2021 19:49

srl295 requested review from markusicu, nedley and macchiati September 20, 2021 19:49

echeran reviewed Sep 27, 2021

View reviewed changes

docs/index.md Outdated Show resolved Hide resolved

docs/index.md Show resolved Hide resolved

echeran reviewed Sep 28, 2021

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

unicodetools/pom.xml Outdated Show resolved Hide resolved

unicodetools/pom.xml Outdated Show resolved Hide resolved

Mavenize: move main classes around

3a22551

Fixes: #64 X-PR-URL: #91

srl295 added a commit that referenced this pull request Sep 28, 2021

Mavenize: move test stuff around

fd0c3a6

- bump surefire and other tests - at least make sure it can compile Fixes: #64 X-PR-URL: #91

srl295 added a commit that referenced this pull request Sep 28, 2021

Mavenize: move data items to the correct spot

eff7ef0

Fixes: #64 X-PR-URL: #91

srl295 force-pushed the srl295/issue64 branch from 51d984b to 21d2d03 Compare September 28, 2021 22:35

echeran previously approved these changes Sep 29, 2021

View reviewed changes

markusicu reviewed Oct 1, 2021

View reviewed changes

srl295 dismissed echeran’s stale review via bc0dfc1 October 1, 2021 19:59

srl295 added 5 commits October 1, 2021 15:15

Mavenize: tests

6c156ed

- tests: rename many test files - pom: bump surefire and other tests - workflow: at least make sure it can compile, build all branches, credentials Fixes: #64 X-PR-URL: #91

Mavenize: move data items to the correct spot

10e6411

Fixes: #64 X-PR-URL: #91

Mavenize: test work

7e9cf93

- move additional test files around Fixes: #64 X-PR-URL: #91

Mavenize: Review Comment Fixes

217a234

- fix broken lines in CharData.java - cleanup maven poms Move more configuration into the top level pom.xml, add some comments Fixes: #64 X-PR-URL: #91

srl295 force-pushed the srl295/issue64 branch from bc0dfc1 to 217a234 Compare October 1, 2021 20:17

srl295 requested a review from markusicu October 1, 2021 20:17

srl295 changed the title ~~Mavenize UnicodeTools (#64) Phase I~~ Mavenize UnicodeTools (#64) Oct 1, 2021

srl295 added a commit that referenced this pull request Oct 1, 2021

adjust workflows to build all branches, not just 'master'/'main'

7e86327

Fixes #58 (note that #91 already corrects the Java part of this)

srl295 mentioned this pull request Oct 1, 2021

adjust workflows to build all branches, not just 'master'/'main' #130

Merged

echeran approved these changes Oct 1, 2021

View reviewed changes

markusicu approved these changes Oct 1, 2021

View reviewed changes

srl295 merged commit 8d6bd2e into main Oct 1, 2021

srl295 deleted the srl295/issue64 branch October 1, 2021 22:08

srl295 mentioned this pull request Oct 4, 2021

doc: improve docs around maven #133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mavenize UnicodeTools (#64) #91

Mavenize UnicodeTools (#64) #91

srl295 commented Jul 5, 2021 •

edited

Loading

srl295 commented Jul 12, 2021

srl295 commented Aug 16, 2021

srl295 commented Sep 20, 2021

srl295 commented Sep 20, 2021 •

edited

Loading

srl295 commented Sep 21, 2021

markusicu commented Sep 27, 2021

srl295 commented Sep 28, 2021 •

edited

Loading

srl295 commented Sep 28, 2021

echeran commented Sep 29, 2021

srl295 commented Sep 29, 2021

srl295 commented Sep 29, 2021

srl295 commented Sep 29, 2021

srl295 commented Sep 29, 2021 •

edited

Loading

markusicu Oct 1, 2021

srl295 Oct 1, 2021

srl295 Oct 1, 2021

srl295 Oct 1, 2021

markusicu Oct 1, 2021

markusicu left a comment

srl295 commented Oct 1, 2021 •

edited

Loading

srl295 commented Oct 1, 2021

srl295 commented Oct 1, 2021

srl295 commented Oct 1, 2021

echeran left a comment

markusicu left a comment

markusicu Oct 1, 2021

Mavenize UnicodeTools (#64) #91

Mavenize UnicodeTools (#64) #91

Conversation

srl295 commented Jul 5, 2021 • edited Loading

srl295 commented Jul 12, 2021

srl295 commented Aug 16, 2021

srl295 commented Sep 20, 2021

srl295 commented Sep 20, 2021 • edited Loading

srl295 commented Sep 21, 2021

markusicu commented Sep 27, 2021

srl295 commented Sep 28, 2021 • edited Loading

srl295 commented Sep 28, 2021

echeran commented Sep 29, 2021

srl295 commented Sep 29, 2021

srl295 commented Sep 29, 2021

srl295 commented Sep 29, 2021

srl295 commented Sep 29, 2021 • edited Loading

markusicu Oct 1, 2021

Choose a reason for hiding this comment

srl295 Oct 1, 2021

Choose a reason for hiding this comment

srl295 Oct 1, 2021

Choose a reason for hiding this comment

srl295 Oct 1, 2021

Choose a reason for hiding this comment

markusicu Oct 1, 2021

Choose a reason for hiding this comment

markusicu left a comment

Choose a reason for hiding this comment

srl295 commented Oct 1, 2021 • edited Loading

srl295 commented Oct 1, 2021

srl295 commented Oct 1, 2021

srl295 commented Oct 1, 2021

echeran left a comment

Choose a reason for hiding this comment

markusicu left a comment

Choose a reason for hiding this comment

markusicu Oct 1, 2021

Choose a reason for hiding this comment

srl295 commented Jul 5, 2021 •

edited

Loading

srl295 commented Sep 20, 2021 •

edited

Loading

srl295 commented Sep 28, 2021 •

edited

Loading

srl295 commented Sep 29, 2021 •

edited

Loading

srl295 commented Oct 1, 2021 •

edited

Loading