Skip to content

Conversation

@macchiati
Copy link
Member

This should work now. You have to supply an environment variable to generate the beta. Added more documentation to the top of Emoji.java.

@macchiati macchiati requested a review from nedley May 2, 2022 14:36
@nedley
Copy link
Contributor

nedley commented May 2, 2022

The changes in Emoji.java are identical to what I had on the branch I sent you and I was setting -Demoji-beta. What changed?

@nedley
Copy link
Contributor

nedley commented May 2, 2022

Is it because I also changed VersionToAge.java? Because I thought I was supposed to…

Comment on lines +44 to +45
* You need a command line variable to generate either the beta version (not yet released) or the abbreviated version (see below).
* <p>Example: -Demoji-beta
Copy link
Member

Choose a reason for hiding this comment

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

The emoji-beta option is already mentioned on https://github.com/unicode-org/unicodetools/blob/main/docs/emoji/index.md
Wouldn't it be better to update that and point from here to there?

Note that there are several docs pages under emoji/ : https://github.com/unicode-org/unicodetools/tree/main/docs/emoji

Comment on lines 81 to 85
/**
* The following is used to generate an abbreviated version of the charts, where only a few rows are produced,
* and all images are replaced by a colored square (small data size).
* This version can be used to do link-checks.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please set your editor to write spaces, not tabs, and fix this comment.

@nedley
Copy link
Contributor

nedley commented May 3, 2022

As I mentioned yesterday, these changes seem like a subset of what I had done on my ned/emoji_15_draft branch so I have to ask: in the generated emoji-data.txt, are you seeing E15.0? and does emoji-test.txt contain 15.0 emoji?

@macchiati
Copy link
Member Author

macchiati commented May 4, 2022 via email

@macchiati macchiati requested a review from markusicu May 6, 2022 04:09
@macchiati
Copy link
Member Author

Made modifications to get the files to build. Updated the emoji/index.md. Still need to build emoji-test.txt, but wanted to get these in first. There is a related PR in the emoji repo.

Copy link
Contributor

@nedley nedley left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to flesh this out. Having the 15.0 additions show up as E0.0 is a problem for the beta but we could fix those by hand if time is running low.

@@ -0,0 +1,126 @@
package org.unicode.propstest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file required for the E15.0 data?


public static <T> void printData() throws IOException {
printData(EmojiDataSourceCombined.EMOJI_DATA);
printData(EmojiDataSourceCombined.EMOJI_DATA_BETA);
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m assuming this was the missing piece?

1F6D1..1F6D2 ; Basic_Emoji ; stop sign # E3.0 [2] (🛑..🛒)
1F6D5 ; Basic_Emoji ; hindu temple # E12.0 [1] (🛕)
1F6D6..1F6D7 ; Basic_Emoji ; hut # E13.0 [2] (🛖..🛗)
1F6DC ; Basic_Emoji ; wireless # E0.0 [1] (🛜)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still showing E0.0.

public static final VersionInfo VERSION0_6 = VersionInfo.getInstance(0,6);
public static final VersionInfo VERSION0_5 = VersionInfo.getInstance(0,5,2);

// ALSO fix VersionToAge.java!
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still relevant?

@nedley
Copy link
Contributor

nedley commented May 6, 2022

@macchiati I will approve your current PRs to not hold things up but at this point we still have issues that are probably blockers for the beta release: charts aside, they are the wrong ages and an incomplete emoji-test.txt. Given your limited availability next week, my proposal is that I make the necessary fixes by hand before trying to debug the tooling but if you know where the problem lies then I will obviously defer to you.

@nedley
Copy link
Contributor

nedley commented May 6, 2022

After scrutinizing the updated comments in the code, it sounds like the emoji-test.txt will be fixed after copying the generated files and re-running the tool?

@macchiati
Copy link
Member Author

Yes, the E0.0 and missing values in the test are fixed by running the tool again. There is one extra step that I now document in the index.md. See how it looks now.

Haven't tracked down the X issue in the charts yet, but I think this part should be good enough to check in (after checking, of course).

And will fix the X later today.

@macchiati macchiati requested a review from nedley May 6, 2022 16:47
@macchiati macchiati merged commit f0fb440 into unicode-org:main May 6, 2022
@macchiati macchiati deleted the 15.0-emoji-versions branch May 6, 2022 18:19
@markusicu
Copy link
Member

@nedley @macchiati sorry for not noticing this before -- but the data files are in the wrong place!
You put them into https://github.com/unicode-org/unicodetools/tree/main/unicodetools/data/emoji/15.0 as in the old days, but we had decided last year to stop doing that, and put development versions into "dev" folders. These should have gone into https://github.com/unicode-org/unicodetools/tree/main/unicodetools/data/emoji/dev

You should also have populated the files in https://github.com/unicode-org/unicodetools/tree/main/unicodetools/data/ucd/dev/emoji
as far as some files are still duplicated in multiple locations.

Please fix!

@nedley
Copy link
Contributor

nedley commented May 11, 2022

#241 (comment)

@markusicu
Copy link
Member

I have sent PR #248 for review to move the emoji files to where they belong, sync the duplicated files, and update the readmes.

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