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.

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

CLDR-10347 show one example for each day period, excluding midnight #3949

Conversation

haytenf
Copy link
Contributor

@haytenf haytenf commented Aug 8, 2024

CLDR-10347 Show one example for each day period. Currently suppresses examples for the day period "midnight" (due to ICU-12278, lack of controlled use of midnight), but handles remaining day periods and adds relevant samples.

Locale Before After
he Screenshot 2024-08-09 at 1 04 25 PM Screenshot 2024-08-09 at 1 04 46 PM Screenshot 2024-08-09 at 1 05 36 PM Screenshot 2024-08-09 at 1 05 52 PM
de Screenshot 2024-08-09 at 12 50 09 PM Screenshot 2024-08-09 at 12 53 02 PM
qu* Screenshot 2024-08-09 at 12 56 29 PM Screenshot 2024-08-09 at 12 57 45 PM

*Only 2 day periods are defined in Quechua, original code produces 3 examples for every locale, so modification results in fewer examples

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@haytenf haytenf force-pushed the CLDR-10347-Show-one-example-for-each-day-period branch from 6aff062 to cf294f0 Compare August 8, 2024 18:46
Copy link
Contributor

@conradarcturus conradarcturus 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 adding these :) It looks great. My only feedback is based on style and I'm myself also pretty new to CLDR so any feedback is optional.

Thanks for including screenshots. A recommendation I have for those screenshots to make it more readable is to 1) zoom in on the specific areas that the difference is (the context is good for like 1 screenshot but then the others are just harder to quickly grok). and 2) use markdown tables like this (see the markdown documentation):

Context Before After
I'm showing X Insert the before image Insert the after image
I'm now showing Y Insert the before image Insert the after image

Regardless none of these comments are blocking so I'm happy to accept if you think its ready -- but I'll leave it open ended in case one of the more seasoned contributors wants to take a look.

srl295
srl295 previously approved these changes Aug 12, 2024
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.

This looks great!

macchiati
macchiati previously approved these changes Aug 12, 2024
@haytenf haytenf marked this pull request as ready for review August 12, 2024 19:52
@macchiati
Copy link
Member

The checks weren't run while in draft, so now running

@haytenf haytenf dismissed stale reviews from macchiati and srl295 via 2d1b73d August 12, 2024 20:57
@srl295
Copy link
Member

srl295 commented Aug 12, 2024

@haytenf the Fixed formatting using spotless commit message needed to be CLDR-10347 Fixed formatting using spotless - If are not familiar with this procedure you can fix it by ONE of these two ways.

  1. https://us-central1-dev-infra-273822.cloudfunctions.net/unicode-github-bot/info/unicode-org/cldr/3949 click squash (this will make it more complicated on your local system) OR
  2. git commit --amend locally - this will let you update the commit message and then run git push -f to push the branch back. i'd recommend this as your local system will be in sync with what's on the server.

@macchiati
Copy link
Member

"jira-ticket — Commit message for 2d1b73d fails validation"
As Steven says.

@haytenf haytenf force-pushed the CLDR-10347-Show-one-example-for-each-day-period branch from 2d1b73d to 2c29a6f Compare August 12, 2024 21:17
@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

Copy link
Contributor

@conradarcturus conradarcturus left a comment

Choose a reason for hiding this comment

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

Awesome! Looks great.

thumbs up cat

@@ -1123,6 +1123,42 @@ private void checkDayPeriod(
checkPathValue(exampleGenerator, path, cldrFile.getStringValue(path), expected);
}

public void TestAllDayPeriods() { // excludes midnight, see ICU-12278
checkDayPeriodsForLocale(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfetto!

@macchiati macchiati merged commit e4264a8 into unicode-org:main Aug 16, 2024
12 checks passed
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.

4 participants