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.

Sign up for GitHub

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

Jump to bottom

Update Unicodejsps to U13.0 and E13.1 #44

Merged
merged 1 commit into from
Jul 17, 2021
Merged

Update Unicodejsps to U13.0 and E13.1 #44

merged 1 commit into from
Jul 17, 2021

Conversation

macchiati
Copy link
Member

@macchiati macchiati commented Feb 26, 2021 •
edited by markusicu
Loading

Adds a few fixes also for UTS #18, and a fix to the layout (when you have too many strings, the generated html was faulty).
Adjusted the target for CopyPropsToUnicodeJsp.java, needed after the mavenizing of UnicodeJsps.
Note that the easiest way to support U13 and E13.1 was to copy some extra files into idna/13.1 and security/13.1

@macchiati
Copy link
Member Author

Changed the method name, and removed the UnicodePropertyPartition. Should be ready now.

srl295
srl295 reviewed Feb 28, 2021
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maven related part LGTM, no comment on the Unicode changes themselves

unicodetools/org/unicode/propstest/CopyPropsToUnicodeJsp.java Outdated Show resolved Hide resolved
@markusicu
Copy link
Member

Pending PR with one or two unresolved comments, and now with merge conflicts. But this still wants to move forward, right?

@macchiati
Copy link
Member Author

macchiati commented May 28, 2021 via email

@srl295 srl295 force-pushed the Update-Unicodejsps-to-U13.0-and-E13.1 branch from e9cf7fb to 7b23830 Compare July 12, 2021 21:18
@srl295
Copy link
Member

srl295 commented Jul 12, 2021

Mark,
I had this same error on your original branch.
I've resolved conflicts (pretty straightforward), but I still get this error in the properties jsp:

Caused by: java.lang.IllegalArgumentException: Only applicable to binary (boolean) properties, not Basic_Emoji
    at org.unicode.jsp.UnicodeProperty.getTrueSet (UnicodeProperty.java:274)
    at org.unicode.jsp.XPropertyFactory.<init> (XPropertyFactory.java:158)
    at org.unicode.jsp.XPropertyFactory.make (XPropertyFactory.java:39)
    at org.unicode.jsp.UnicodeUtilities.<clinit> (UnicodeUtilities.java:247)
    at org.unicode.jsp.UnicodeJsp.showPropsTable (UnicodeJsp.java:168)
    at org.apache.jsp.properties_jsp._jspService (properties_jsp.java:208)

@srl295 srl295 force-pushed the Update-Unicodejsps-to-U13.0-and-E13.1 branch from 7b23830 to af2431d Compare July 12, 2021 21:20
@srl295
Copy link
Member

srl295 commented Jul 12, 2021

Mark,
I had this same error on your original branch.
I've resolved conflicts (pretty straightforward), but I still get this error in the properties jsp:

Caused by: java.lang.IllegalArgumentException: Only applicable to binary (boolean) properties, not Basic_Emoji
    at org.unicode.jsp.UnicodeProperty.getTrueSet (UnicodeProperty.java:274)
    at org.unicode.jsp.XPropertyFactory.<init> (XPropertyFactory.java:158)
    at org.unicode.jsp.XPropertyFactory.make (XPropertyFactory.java:39)
    at org.unicode.jsp.UnicodeUtilities.<clinit> (UnicodeUtilities.java:247)
    at org.unicode.jsp.UnicodeJsp.showPropsTable (UnicodeJsp.java:168)
    at org.apache.jsp.properties_jsp._jspService (properties_jsp.java:208)

Basic_Emoji is a DelayedUnicodeProperty, of type UNKNOWN.

@srl295 srl295 requested review from markusicu and srl295 July 15, 2021 20:22
@srl295
Copy link
Member

srl295 commented Jul 15, 2021

@macchiati i know you can't review since you opened it, but let me knwo if you have any comments

@srl295 srl295 mentioned this pull request Jul 15, 2021
Merged
@markusicu
Copy link
Member

Basic_Emoji is a DelayedUnicodeProperty, of type UNKNOWN.

These are boolean properties of strings, and outside of the UCD, so generic code that gets the type from PropertyAliases.txt won't know what these are. We probably need to hardcode their types into the tools until we can get something better into the Unicode data files. (Mark and I have related action items that we haven't gotten to yet.)

markusicu
markusicu reviewed Jul 16, 2021
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks mostly plausible; please also take a look at a couple of older comments and fix or otherwise resolve

unicodetools/data/idna/13.1.0/ReadMe.txt Show resolved Hide resolved
unicodetools/org/unicode/props/ExtraPropertyValueAliases.txt Show resolved Hide resolved
unicodetools/org/unicode/props/UcdPropertyValues.java Outdated Show resolved Hide resolved
unicodetools/org/unicode/propstest/CheckEmojiProps2.java Outdated Show resolved Hide resolved
unicodetools/org/unicode/text/utility/Settings.java Show resolved Hide resolved
@srl295
Copy link
Member

srl295 commented Jul 16, 2021

looks mostly plausible; please also take a look at a couple of older comments and fix or otherwise resolve

Ok, will take a spin through. There are a lot of "13.1" related changes.

@srl295 srl295 mentioned this pull request Jul 16, 2021
Open
@srl295 srl295 requested a review from markusicu July 17, 2021 00:01
@srl295 srl295 requested a review from markusicu July 17, 2021 00:13
@markusicu
Copy link
Member

I think I am out of comments. Could you please squash?

@macchiati @markusicu @srl295
Update UnicodeJsps to U13.0 and E13.1
5ffba89 
- a few fixes also for UTS unicode-org#18.
- Note that the easiest way to support U13 and E13.1 was to copy some extra
files into idna/13.1 and security/13.1 (to revisit in unicode-org#100)
- Clarify where getTrueSet() can't be called
- Work around getTrueSet() exception for UNKNOWN type data
- Add CheckEmojiProps2.java (work around name conflict)
- improve tool output

Co-authored-by: Markus Scherer <markus.icu@gmail.com>
Co-authored-by: Steven R. Loomis <srl295@gmail.com>
@srl295 srl295 force-pushed the Update-Unicodejsps-to-U13.0-and-E13.1 branch from 0a6eb08 to 5ffba89 Compare July 17, 2021 00:26
@srl295 srl295 self-requested a review July 17, 2021 00:26
@srl295
Copy link
Member

srl295 commented Jul 17, 2021

Squashed to 1. Verified JSP works as well after squash.

markusicu
markusicu approved these changes Jul 17, 2021
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm tnx!

@srl295 srl295 merged commit 84a9500 into unicode-org:main Jul 17, 2021
@srl295
Copy link
Member

srl295 commented Jul 17, 2021

Will make a release and deploy Monday.

@macchiati macchiati mentioned this pull request Dec 12, 2023
Open
@markusicu markusicu mentioned this pull request Jan 30, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusicu markusicu markusicu approved these changes

@srl295 srl295 Awaiting requested review from srl295

Assignees

@srl295 srl295

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@macchiati @markusicu @srl295