Skip to content

move UnicodeProperty so it doesn't shadow CLDRs #214

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

Merged
merged 4 commits into from
Apr 7, 2022
Merged

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Mar 24, 2022

For: #66

while working on #186 I found some tools wouldn't even work for me due to the shadowed classes issue discussed in #66

This untangles things a bit… while making other things a little more tangled.

Draft because I'm not sure about all of this, but it does pass tests, and GenerateIdna actually runs again instead of crashing.

srl295 added 4 commits March 24, 2022 09:22
- also move some code out of UnicodeJsps into org.unicode.jsp

Relates to #66
…ackage

- also move some code out of UnicodeJsps into org.unicode.jsp

Relates to #66
- deprecate org.unicode.jsp.UnicodeProperty

Relates to #66
- deprecate org.unicode.jsp.UnicodeProperty

Relates to #66
@srl295 srl295 self-assigned this Mar 24, 2022
@markusicu markusicu changed the title move UnicodeProperties so it doesn't shadow CLDRs move UnicodeProperty so it doesn't shadow CLDRs Mar 24, 2022
@macchiati
Copy link
Member

macchiati commented Mar 24, 2022 via email

@markusicu
Copy link
Member

Mark:

I think the very best approach would be to remove UnicodeProperty from CLDR if possible. There are however lots of direct and indirect dependencies. Second best would be to merge anything useful from the unicodetools one into the CLDR one, and delete the unicodetools one.

Yes, all as discussed in the top description of issue #66.
And, as it says there, the unicodetools code has been running with the CLDR repo version which is newer and has more recent changes.

I agree with moving the unicodetools version to a different package, to avoid the shadowing, but I strongly suspect that the version there needs to be mostly a copy of the CLDR version, in order to preserve behavior.

Steven:

I'm not sure we've been using the CLDR version consistently. I think it may be hit or miss depending on how it is packaged or loaded.

My comment from last year says that the CLDR version has a different signature. Either the Unicode Tools rely on that now, or they don't call anything where the API has changed.

From #66:

Until 1.5 years ago they were identical, but they have been diverging and are now no longer even interface-compatible. The CLDR version is the newer one.

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple of questions.

Were the moved files like BagFormatter just copied, or were there more substantive changes?
Why was "final" removed from a number of declarations in unicodetools/src/main/java/org/unicode/jsp/UnicodeRegex.java? Was that because of merging with another version?

@srl295
Copy link
Member Author

srl295 commented Mar 24, 2022

Looks good to me. A couple of questions.

Were the moved files like BagFormatter just copied, or were there more substantive changes?

The ones from CLDR that are completely new were not changed. I can enumerate.

Why was "final" removed from a number of declarations in unicodetools/src/main/java/org/unicode/jsp/UnicodeRegex.java? Was that because of merging with another version?

Merging between the unicodetools and the jsp version.

@srl295 srl295 marked this pull request as ready for review March 25, 2022 03:43
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 45f84f6 into main Apr 7, 2022
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.

3 participants