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

CLDR-15698 Time examples < 10 and > 12 #3895

Merged
merged 2 commits into from
Sep 11, 2024
Merged

CLDR-15698 Time examples < 10 and > 12 #3895

merged 2 commits into from
Sep 11, 2024

Conversation

e-ikwut
Copy link
Contributor

@e-ikwut e-ikwut commented Jul 23, 2024 •
edited
Loading

output.txt

before:
image

after:
image

CLDR-15698

ALLOW_MANY_COMMITS=true

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@e-ikwut e-ikwut changed the title CLDR-15698 CLDR-15698 Time examples < 10 and > 12 Jul 23, 2024
btangmu
btangmu reviewed Jul 23, 2024
View reviewed changes
tools/cldr-code/src/main/java/org/unicode/cldr/test/ExampleGenerator.java Outdated
}

return formatExampleList(examples.toArray(new String[0]));
} else {
String id = parts.findAttributeValue("dateFormatItem", "id");
if ("NEW".equals(id) || value == null) {
return startItalicSymbol + "n/a" + endItalicSymbol;
return startItalicSymbol + "n/a" + endItalicSymbol; // 15698: doesn't seem relevant--meaning??
Copy link
Member

Choose a reason for hiding this comment

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

It's a mystery to me as well...

Copy link
Member

Choose a reason for hiding this comment

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

The id being "NEW" seems odd to me as well; I just checked the DTD, and it isn't permitted:

<!ATTLIST dateFormatItem id CDATA #REQUIRED >
<!--@MATCH:regex/((E|EEEE)?(H|HH|h|hh|Bh)(m|ms|mm|mmss)?(Z|z|zzzz|v|vvvv)?)|(ms|mmss)|((G|GGGGG)?(y|yy|yyyy)((M{1,4}((E|EEEE|cccc)?(d|dd))?)|(w|Q|QQQ|QQQQ))?)|(U(M|MMM)d?)|(M{1,4}(((E|EEEE|cccc)?(d|dd))|W)?)|((E|EEEE)?d)|(E|EEEE)-->

So that looks obsolete, and we can probably remove just that check.

I wouldn't remove the value==null check, however, without verifying that it can't occur at this point in the code.

@macchiati
Copy link
Member

The output.txt is useful, but best to also have before and after screen shots also. And for the text output, also best to have the before and after.

macchiati
macchiati requested changes Jul 25, 2024
View reviewed changes
tools/cldr-code/src/main/java/org/unicode/cldr/test/ExampleGenerator.java Outdated
@@ -2556,7 +2560,7 @@ private String handleDateFormatItem(String xpath, String value, boolean showCont
setBackground("'" + timeRange + "'"),
setBackground("'" + dfResult + "'")
}));
examples.add(dtf.format(DATE_SAMPLE));
examples.add(dtf.format(DATE_SAMPLE)); // 15698: don't need two times for range
Copy link
Member

Choose a reason for hiding this comment

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

You appear to be leaving comments in the code where you think that something is superfluous. Better to use github comments for that, rather than cluttering up the code.

Copy link
Member

Choose a reason for hiding this comment

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

If it were adding duplicate examples, I think we would have seen than in the results. So it appears that the dtf is changing sufficiently that duplicates are not being added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, those were notes to self while I worked (what I meant by that comment was that I don't think I need to add an extra example in that case). Understood.

tools/cldr-code/src/main/java/org/unicode/cldr/test/ExampleGenerator.java Outdated
}

return formatExampleList(examples.toArray(new String[0]));
} else {
String id = parts.findAttributeValue("dateFormatItem", "id");
if ("NEW".equals(id) || value == null) {
return startItalicSymbol + "n/a" + endItalicSymbol;
return startItalicSymbol + "n/a" + endItalicSymbol; // 15698: doesn't seem relevant--meaning??
Copy link
Member

Choose a reason for hiding this comment

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

The id being "NEW" seems odd to me as well; I just checked the DTD, and it isn't permitted:

<!ATTLIST dateFormatItem id CDATA #REQUIRED >
<!--@MATCH:regex/((E|EEEE)?(H|HH|h|hh|Bh)(m|ms|mm|mmss)?(Z|z|zzzz|v|vvvv)?)|(ms|mmss)|((G|GGGGG)?(y|yy|yyyy)((M{1,4}((E|EEEE|cccc)?(d|dd))?)|(w|Q|QQQ|QQQQ))?)|(U(M|MMM)d?)|(M{1,4}(((E|EEEE|cccc)?(d|dd))|W)?)|((E|EEEE)?d)|(E|EEEE)-->

So that looks obsolete, and we can probably remove just that check.

I wouldn't remove the value==null check, however, without verifying that it can't occur at this point in the code.

@macchiati
Copy link
Member

There is a commit message that fails validation. The message must be of the form:

CLDR-15698

eg

CLDR-15698 Time examples < 10 and > 12

The "Details" button on the "jira-ticket — Commit message for 41fe3cb fails validation" gives you a way to fix that, by squashing. Or you can make a new PR. Let us know on slack if you have trouble with that.

e-ikwut pushed a commit to e-ikwut/cldr that referenced this pull request Jul 26, 2024
CLDR-15698 Time examples < 10 and > 12
4083bf7 
See unicode-org#3895
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@macchiati
Copy link
Member

The screenshots look good.

@AEApple AEApple requested a review from macchiati July 30, 2024 18:14
@AEApple
Copy link
Contributor

AEApple commented Jul 31, 2024

Will the same examples show in the Info Panel?

@e-ikwut
Copy link
Contributor Author

e-ikwut commented Jul 31, 2024

Will the same examples show in the Info Panel?

Yes, the same set shows in the pop-up and the info panel.

macchiati
macchiati previously approved these changes Aug 19, 2024
View reviewed changes
@macchiati
Copy link
Member

Once you are ready for review, change the status to "Ready for Review". I approved, but you'll have to fix the conflicts and ask for a re-review.

@e-ikwut e-ikwut closed this Aug 20, 2024
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/test/ExampleGenerator.java is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@e-ikwut e-ikwut reopened this Aug 20, 2024
e-ikwut added a commit to e-ikwut/cldr that referenced this pull request Aug 20, 2024
@e-ikwut
CLDR-15698 Time examples < 10 and > 12
99cea22 
See unicode-org#3895
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@e-ikwut e-ikwut marked this pull request as ready for review August 20, 2024 21:47
@e-ikwut e-ikwut requested a review from macchiati August 20, 2024 21:47
macchiati
macchiati previously approved these changes Aug 23, 2024
View reviewed changes
@e-ikwut e-ikwut mentioned this pull request Aug 28, 2024
Draft
1 task
@srl295 srl295 self-assigned this Sep 3, 2024
@srl295
Copy link
Member

srl295 commented Sep 3, 2024

@e-ikwut needs mvn spotless:apply can you do that? make sure the commit is formatted properly

srl295
srl295 previously approved these changes Sep 3, 2024
View reviewed changes
@echeran echeran dismissed stale reviews from srl295 and macchiati via f2304f4 September 11, 2024 00:08
@sffc sffc mentioned this pull request Sep 11, 2024
Merged
1 task
@sffc sffc merged commit d120055 into unicode-org:main Sep 11, 2024
12 checks passed
haytenf pushed a commit to haytenf/cldr that referenced this pull request Sep 17, 2024
@e-ikwut @echeran @haytenf
CLDR-15698 Time examples < 10 and > 12 (unicode-org#3895)
22b7946 
Co-authored-by: Elango Cheran <elango@unicode.org>
conradarcturus pushed a commit that referenced this pull request Sep 25, 2024
@e-ikwut @echeran @conradarcturus
CLDR-15698 Time examples < 10 and > 12 (#3895)
6b2f994 
Co-authored-by: Elango Cheran <elango@unicode.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btangmu btangmu btangmu left review comments

@sffc sffc sffc approved these changes

@macchiati macchiati macchiati left review comments

@srl295 srl295 srl295 left review comments

@pedberg-icu pedberg-icu Awaiting requested review from pedberg-icu

Assignees

@srl295 srl295

@e-ikwut e-ikwut

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@e-ikwut @CLAassistant @macchiati @AEApple @srl295 @sffc @btangmu @echeran