Teach length::Bag how to switch hour cycles #840

gregtatum · 2021-06-30T21:43:05Z

Edit: This is now ready for review. Please ignore the first commit as it is #841, which I plan on landing first.

Resolves #671

Time zones are going to be required for full support here, as the generated patterns do not use the time zones. I commented out a check for unsupported symbols temporarily to make everything work, but the results can be seen in the generated data. What you'll see here is that I opted for generated 2 new sets of patterns, one for h11/h12, and one for h23/h24. This is in addition to the existing styles. the existing time style represents the localizer's choice for the best hour cycle. The time_h11_h12 and time_h23_24 represent the patterns for those specific preferences. There is a bit of duplicated data doing it this way, but I felt it was more correct as it allows localizers to specify any preference for hour cycle, and it also allows for specific overrides.

There is an assumption that a h11 and h12 symbol can be swapped out correctly in a pattern, as well as a h23 and h24 can be swapped. However, h11 cannot be swapped with with h23 etc.

I do the pattern generation code in the provider to pre-generate the data, rather than requiring the pattern generator to be run at runtime for styles. I anticipate a small regression to memory for this approach, as a trade-off of a bigger one if this pattern manipulation was done at runtime.

jira-pull-request-webhook · 2021-06-30T21:55:01Z

Notice: the branch changed across the force-push!

components/datetime/src/provider/helpers.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

codecov-commenter · 2021-06-30T21:55:01Z

Codecov Report

Merging #840 (8e56e10) into main (cd4b7c5) will increase coverage by 0.53%.
The diff coverage is 95.18%.

@@            Coverage Diff             @@
##             main     #840      +/-   ##
==========================================
+ Coverage   74.25%   74.78%   +0.53%     
==========================================
  Files         211      207       -4     
  Lines       12416    12507      +91     
==========================================
+ Hits         9219     9353     +134     
+ Misses       3197     3154      -43

Impacted Files	Coverage Δ
components/datetime/src/options/length.rs	`25.00% <ø> (ø)`
components/datetime/src/options/preferences.rs	`58.33% <ø> (ø)`
components/icu/src/lib.rs	`100.00% <ø> (ø)`
components/datetime/src/pattern/mod.rs	`67.69% <83.33%> (+0.75%)`	⬆️
...nents/datetime/src/pattern/transform_hour_cycle.rs	`88.23% <88.23%> (ø)`
provider/cldr/src/transform/dates/patterns.rs	`85.20% <96.36%> (+5.20%)`	⬆️
components/datetime/src/skeleton.rs	`85.14% <97.05%> (+3.17%)`	⬆️
components/datetime/src/provider/gregory.rs	`64.00% <100.00%> (+0.98%)`	⬆️
components/datetime/src/provider/helpers.rs	`82.78% <100.00%> (+2.41%)`	⬆️
provider/testdata/src/metadata.rs
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd4b7c5...8e56e10. Read the comment docs.

coveralls · 2021-06-30T21:56:11Z

Pull Request Test Coverage Report for Build fa138f514a7750f40317feec08c698f8a8540a5c-PR-840

178 of 187 (95.19%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.4%) to 74.842%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/datetime/src/pattern/mod.rs	5	6	83.33%
components/datetime/src/skeleton.rs	66	68	97.06%
provider/cldr/src/transform/dates/patterns.rs	53	55	96.36%
components/datetime/src/pattern/transform_hour_cycle.rs	30	34	88.24%

Files with Coverage Reduction	New Missed Lines	%
components/datetime/src/options/length.rs	1	25.0%

Totals
Change from base Build cd4b7c536c0b573aa68510bc8e1776fa78b1c0fd:	0.4%
Covered Lines:	9484
Relevant Lines:	12672

💛 - Coveralls

jira-pull-request-webhook · 2021-07-01T16:40:23Z

Notice: the branch changed across the force-push!

components/datetime/src/fields/symbols.rs is now changed in the branch
components/datetime/src/options/components.rs is now changed in the branch
components/datetime/src/options/preferences.rs is now changed in the branch
components/datetime/src/pattern/mod.rs is different
components/datetime/src/pattern/parser.rs is now changed in the branch
components/datetime/src/provider/gregory.rs is now changed in the branch
components/datetime/src/provider/helpers.rs is different
components/datetime/src/skeleton.rs is different
components/datetime/tests/datetime.rs is different
components/datetime/tests/fixtures/tests/components-exact-matches.json is now changed in the branch
components/datetime/tests/fixtures/tests/lengths_with_zones_from_pdt.json is now changed in the branch
components/datetime/tests/fixtures/tests/skeletons.bin is now changed in the branch
provider/cldr/src/transform/dates/patterns.rs is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/ar-EG.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/ar.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/bn.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/ccp.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/en-001.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/en-ZA.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/en.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/es-AR.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/es.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/fr.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/ja.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/ru.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/sr-Cyrl.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/sr-Latn.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/sr.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/th.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/tr.json is now changed in the branch
provider/testdata/data/json/date_patterns/gregory@1/und.json is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2021-07-01T16:42:58Z

Notice: the branch changed across the force-push!

components/datetime/src/skeleton.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

zbraniecki · 2021-07-01T16:58:41Z

Your new sets - time_h11_h12 and time_h23_h24 are always a pair of one duplicated and one non-duplicated. Could we skip the duplicated one?

gregtatum · 2021-07-01T19:50:40Z

I took a stab at de-duplicating it. The risk is that lengths mix a h11h12 with a h23h24. In that case I have the provider transform panic. I checked the CLDR data, and no locales are currently doing this. It saves 120 bytes but adds a bit more complexity to the implementation.

commit	total allocations	max memory
de-duplicated	53,718 bytes (+197)	26,492 bytes (+197)
duplicated	53,838 bytes (+317)	26,612 bytes (+317)
main branch	53,521 bytes	26,295 bytes

components/datetime/src/provider/gregory.rs

components/datetime/src/pattern/mod.rs

components/datetime/src/skeleton.rs

… cycle transform code

jira-pull-request-webhook · 2021-07-02T17:04:46Z

Notice: the branch changed across the force-push!

components/datetime/src/fields/symbols.rs is no longer changed in the branch
components/datetime/src/options/components.rs is no longer changed in the branch
components/datetime/src/pattern/mod.rs is different
components/datetime/src/pattern/parser.rs is no longer changed in the branch
components/datetime/src/pattern/transform_hour_cycle.rs is different
components/datetime/src/skeleton.rs is different
components/datetime/tests/fixtures/tests/components-exact-matches.json is no longer changed in the branch
components/datetime/tests/fixtures/tests/lengths_with_zones_from_pdt.json is no longer changed in the branch
components/datetime/tests/fixtures/tests/skeletons.bin is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

experimental/calendar/README.md

sffc

This looks good! I like how you were able to support the hour cycle preferences at runtime with a very small amount of code, front-loading most of the work to the transformer.

Just some nits.

components/datetime/src/pattern/mod.rs

components/datetime/Cargo.toml

gregtatum · 2021-07-22T14:12:53Z

I did an ugly fix due to merging in #832. The widths for hour cycles were expanding in ways I didn't want them to. The get_best_available_format_pattern wasn't really giving me the results I wanted, as it would respect my width choice. I am debating whether it would be better to have get_best_available_format_pattern not take fields: &[Field] to operate on, but rather a special subset of the fields, defined as fields: &[SkeletonField].

The skeleton fields do not match 1:1 with pattern fields, and I feel like I'm constantly designing around that. I wonder if I shouldn't file a follow-up issue to tackle this.

jira-pull-request-webhook · 2021-07-22T14:36:04Z

Notice: the branch changed across the force-push!

components/datetime/src/pattern/transform_hour_cycle.rs is different
components/datetime/src/provider/helpers.rs is different
components/datetime/src/skeleton.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

gregtatum · 2021-07-22T14:37:37Z

Apologies for the force push, but I consolidated my changes after the approval to make it easier to review. The changes are only e7ee7e3.

sffc

Seems good; I defer to Zibi to review the details of the business logic

robertbastian · 2023-03-14T16:46:17Z

There is an assumption that a h11 and h12 symbol can be swapped out correctly in a pattern, as well as a h23 and h24 can be swapped. However, h11 cannot be swapped with with h23 etc.

This assumption seems to break in CLDR 43

thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `H23H24`,
 right: `H11H12`: A locale contained a mix of coarse hour cycle types', provider/datagen/src/transform/cldr/datetime/patterns.rs:140:17

sffc · 2023-03-16T21:51:40Z

Possibly related:

tc39/ecma402#758

sffc · 2023-03-20T08:23:02Z

@robertbastian What is the locale causing the error? What change in CLDR between 42 and 43 is causing the error? That information can help us decide the best approach to fixing it.

robertbastian · 2023-03-20T10:45:34Z

It's byn (https://github.com/unicode-org/cldr-json/blob/maint/maint-43/cldr-json/cldr-dates-full/main/byn/ca-gregorian.json#L285-L289) and ssy (https://github.com/unicode-org/cldr-json/blob/maint/maint-43/cldr-json/cldr-dates-full/main/ssy/ca-gregorian.json#L285-L289)

robertbastian · 2023-03-21T11:42:03Z

Do we consider this a CLDR bug, or is this mixed assumption something we can drop?

srl295 · 2023-03-21T17:27:36Z

As with some other issues, byn and ssy aren’t at basic level yet. So in some sense, it’s not a bug, because the locale coverage mechanism has already warned you about the locale.

Looking at these specifically, the issue is that of the four timeFormats, two or three of them are unconfirmed—they need more votes. By default, unconfirmed are dropped from cldr-json. Therefore the patterns with H fall through from root.

I added a comment to https://unicode-org.atlassian.net/browse/CLDR-11836?focusedCommentId=168839 for this issue under logical grouping errors.

That said, this is also probably a reasonable assumption that hourcycles are consistent. I don't know off hand whether there's a separate test or ticket for that. That would be fine to file as a ticket, for next time (Cross reference the logical group issue)

gregtatum mentioned this pull request Jun 30, 2021

Hour Cycle Preferences ignored #671

Closed

gregtatum force-pushed the hour-cycle branch from 3bbccbb to 8b6b665 Compare June 30, 2021 21:54

gregtatum force-pushed the hour-cycle branch from 8b6b665 to 3124b0b Compare July 1, 2021 16:40

gregtatum force-pushed the hour-cycle branch from 3124b0b to 30eca6a Compare July 1, 2021 16:42

gregtatum marked this pull request as ready for review July 1, 2021 16:53

gregtatum requested review from sffc and zbraniecki as code owners July 1, 2021 16:53

sffc requested changes Jul 2, 2021

View reviewed changes

gregtatum requested a review from a team as a code owner July 2, 2021 16:50

gregtatum added 6 commits July 2, 2021 11:59

Teach length::Bag how to switch hour cycles

cf29e4f

De-duplicate the hour cycle in the length data

4e891be

Rename flexible to coarse, and restructure provider data

d6d0575

Break out hour cycle into its own module

4529f36

Add a feature flag provider_transform_utils, and apply it to the hour…

4c6f00c

… cycle transform code

Correct H23 as the stored symbol for skeletons

dc73213

gregtatum force-pushed the hour-cycle branch from 32d086a to dc73213 Compare July 2, 2021 17:04

Fix CI by running cargo make generate-readmes

b80883a

gregtatum commented Jul 2, 2021

View reviewed changes

experimental/calendar/README.md Outdated Show resolved Hide resolved

gregtatum requested a review from sffc July 2, 2021 17:09

This was referenced Jul 2, 2021

Pattern matching fails in the components bag when matching h11 vs h12, or h23 vs h24 #843

Closed

Correctly apply the hour cycle in the components::Bag #846

Merged

sffc reviewed Jul 2, 2021

View reviewed changes

components/datetime/src/pattern/mod.rs Outdated Show resolved Hide resolved

components/datetime/Cargo.toml Outdated Show resolved Hide resolved

gregtatum dismissed stale reviews from sffc and zbraniecki via 5963720 July 22, 2021 13:55

Work around width expansion issue

e7ee7e3

gregtatum force-pushed the hour-cycle branch from e6e65a7 to e7ee7e3 Compare July 22, 2021 14:36

gregtatum requested a review from sffc July 22, 2021 15:50

sffc previously approved these changes Jul 22, 2021

View reviewed changes

gregtatum added 2 commits July 23, 2021 08:49

Merge in main

43ee215

Update comment to be more general

816746c

gregtatum dismissed sffc’s stale review via 816746c July 23, 2021 13:55

zbraniecki previously approved these changes Jul 27, 2021

View reviewed changes

Merge branch 'main' into hour-cycle

b978ed3

gregtatum dismissed zbraniecki’s stale review via b978ed3 July 28, 2021 17:23

zbraniecki previously approved these changes Jul 28, 2021

View reviewed changes

Add std as a feature requirement for provider_transform_internals

8e56e10

gregtatum dismissed zbraniecki’s stale review via 8e56e10 July 28, 2021 18:04

Manishearth approved these changes Jul 28, 2021

View reviewed changes

zbraniecki approved these changes Jul 28, 2021

View reviewed changes

gregtatum merged commit 3d49bb1 into unicode-org:main Jul 28, 2021

This was referenced Mar 14, 2023

CLDR 43 Beta 3 #3185

Merged

CLDR 43 integration #3181

Closed

robertbastian mentioned this pull request Mar 21, 2023

Support mixed hour cycles #3212

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teach length::Bag how to switch hour cycles #840

Teach length::Bag how to switch hour cycles #840

gregtatum commented Jun 30, 2021 •

edited

Loading

jira-pull-request-webhook bot commented Jun 30, 2021

codecov-commenter commented Jun 30, 2021 •

edited

Loading

coveralls commented Jun 30, 2021 •

edited

Loading

jira-pull-request-webhook bot commented Jul 1, 2021

jira-pull-request-webhook bot commented Jul 1, 2021

zbraniecki commented Jul 1, 2021

gregtatum commented Jul 1, 2021

jira-pull-request-webhook bot commented Jul 2, 2021

sffc left a comment

gregtatum commented Jul 22, 2021

jira-pull-request-webhook bot commented Jul 22, 2021

gregtatum commented Jul 22, 2021

sffc left a comment

robertbastian commented Mar 14, 2023

sffc commented Mar 16, 2023

sffc commented Mar 20, 2023

robertbastian commented Mar 20, 2023

robertbastian commented Mar 21, 2023

srl295 commented Mar 21, 2023

Teach length::Bag how to switch hour cycles #840

Teach length::Bag how to switch hour cycles #840

Conversation

gregtatum commented Jun 30, 2021 • edited Loading

jira-pull-request-webhook bot commented Jun 30, 2021

codecov-commenter commented Jun 30, 2021 • edited Loading

Codecov Report

coveralls commented Jun 30, 2021 • edited Loading

Pull Request Test Coverage Report for Build fa138f514a7750f40317feec08c698f8a8540a5c-PR-840

💛 - Coveralls

jira-pull-request-webhook bot commented Jul 1, 2021

jira-pull-request-webhook bot commented Jul 1, 2021

zbraniecki commented Jul 1, 2021

gregtatum commented Jul 1, 2021

jira-pull-request-webhook bot commented Jul 2, 2021

sffc left a comment

Choose a reason for hiding this comment

gregtatum commented Jul 22, 2021

jira-pull-request-webhook bot commented Jul 22, 2021

gregtatum commented Jul 22, 2021

sffc left a comment

Choose a reason for hiding this comment

robertbastian commented Mar 14, 2023

sffc commented Mar 16, 2023

sffc commented Mar 20, 2023

robertbastian commented Mar 20, 2023

robertbastian commented Mar 21, 2023

srl295 commented Mar 21, 2023

gregtatum commented Jun 30, 2021 •

edited

Loading

codecov-commenter commented Jun 30, 2021 •

edited

Loading

coveralls commented Jun 30, 2021 •

edited

Loading