Year of week-of-year support #1206

mildgravitas · 2021-10-25T15:27:50Z

Implements year of year-of-week support for #488 and:

Adds a dedicated Year enum to datetime::options::components::Bag.
Loosens get_best_available_format_pattern() & adjust_pattern_field_lengths() to match FieldSymbol enums but not their data. This is necessary for components::Year matching to work & also improves h12<->h23 coerced time patterns which previously dropped the timezone.

zbraniecki · 2021-10-25T17:52:59Z

@mildgravitas this PR will have to wait for #1198 which will bitrot it. Please, wait for that to land and rebase on top

Manishearth · 2021-10-25T18:47:57Z

(CI is failing due to a bug in CI that is fixed on main, feel free to ignore it)

gregtatum · 2021-10-25T21:14:40Z

I'm requested for review on here, and the PR is marked as draft. Are you looking for early feedback @mildgravitas or full review?

Manishearth · 2021-10-25T21:59:36Z

@gregtatum I think Github autorequested review here? Usually when it says "as code owners" it's because GitHub picked reviewers automatically

jira-pull-request-webhook · 2021-10-29T15:02:15Z

Notice: the branch changed across the force-push!

components/datetime/src/fields/symbols.rs is different
components/datetime/src/format/datetime.rs is different
components/datetime/src/options/components.rs is different
components/datetime/src/skeleton/helpers.rs is different
components/datetime/src/skeleton/mod.rs is different
provider/testdata/data/json/datetime/gregory_lengths@1/fr.json is different
provider/testdata/data/json/datetime/gregory_lengths@1/ru.json is different
provider/testdata/data/json/datetime/gregory_lengths@1/th.json is different
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

mildgravitas · 2021-10-29T15:38:24Z

#1198 was merged & I've rebased onto 6fba6e5 -> switch back from draft.

zbraniecki

Left some comments, nothing major but would like to re-review based on the responses.

I'd also like @gregtatum to review the discriminant_idx and its impact on skeleton selection.

zbraniecki · 2021-10-29T16:54:00Z

components/datetime/src/options/components.rs

+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub enum Year {
+    /// The numeric value of the year, such as "2021".
+    #[cfg_attr(feature = "serde", serde(rename = "numeric"))]


question: why are you renaming it here?

For compatibility with existing usage in test fixtures. I can remove the renames & update the latter if preferable.

please do. ICU4X data shouldn't use renames to conform to anything external. And test fixtures should be updated when the values change.

zbraniecki · 2021-10-29T16:55:21Z

components/datetime/src/options/components.rs

+    /// The numeric value of the year in "week-of-year", such as "2021".
+    NumericWeekOf,
+    /// The two-digit value of the year in "week-of-year", such as "21".
+    TwoDigitWeekOf,


issue: the TwoDigit and TwoDigitWeekOf and Numeric and NumericWeekOf comments are exactly the same. They don't help understand what's the difference and why we need both.

They're not equivalent but the difference is subtle. I've added examples to make it more salient.

nit: Thanks, that's helpful, but the reader still has to derive an implicit information about the actual value. Could you extend the comments for NumericWeekOf and TwoDigitWeekOf to:

/// The numeric value of the year in "week-of-year", such as "2019" of the "week 01 of 2019" for the /// week of 2018-12-31 according to the ISO calendar.

zbraniecki · 2021-10-29T17:03:40Z

provider/testdata/data/json/datetime/gregory_lengths@1/ar-EG.json

-    "full": "HH:mm:ss",
-    "long": "HH:mm:ss",
+    "full": "HH:mm:ss v",
+    "long": "HH:mm:ss v",


question: why are you changing test data JSON files?

Ah, that's a consequence of the discriminant_cmp() changes to skeleton matching: the changed patterns are 12<->24 hour translations of CLDR's full or long time patterns. The algorithm (from components/datetime/src/pattern/hour_cycle.rs) here is:

Take the original pattern & swap 12<->24 hour symbols. e.g. 'h:mm:ss a zzzz' -> 'H:mm:ss a zzzz'.

Turn the pattern into a skeleton. e.g. 'H:mm:ss a zzzz' -> 'Hmsv'.

Call skeleton::create_best_pattern_for_fields() to find a matching pattern.

Previously 'v' & 'z' were considered distinct so create_best_pattern_for_fields() would settle for a partial match of 'Hmsv' against 'Hms'. Now it finds an exact match.

It's possible to cancel this change by altering discriminant_cmp() to compare the inner data for TimeZone, but given that this is arguably more correct I left it in.

Thanks for the fix!

jira-pull-request-webhook · 2021-11-02T14:29:28Z

Notice: the branch changed across the force-push!

provider/testdata/data/json/datetime/gregory_lengths@1/es-AR.json is different
provider/testdata/data/json/datetime/gregory_lengths@1/es.json is different
provider/testdata/data/json/datetime/gregory_lengths@1/sr-Cyrl.json is different
provider/testdata/data/json/datetime/gregory_lengths@1/sr-Latn.json is different
provider/testdata/data/json/datetime/gregory_lengths@1/sr.json is different
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

mildgravitas · 2021-11-02T14:29:42Z

Rebased onto dc5ff31

zbraniecki · 2021-11-02T14:58:10Z

This looks good to me - with the changes I requested.

I'd like @nordzilla or @gregtatum to review the skeleton's changes in particular before we merge this.

jira-pull-request-webhook · 2021-11-02T17:09:03Z

Notice: the branch changed across the force-push!

provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

mildgravitas · 2021-11-02T17:09:18Z

Rebased onto 0c855b1

nordzilla

Looks good to me overall, though @gregtatum is certainly more of an authority on the skeletons and patterns here, so I'd still like to wait for his review.

I had a couple things I'd like to see changed with regard to style.

components/datetime/src/skeleton/helpers.rs

components/datetime/benches/fixtures/tests/components.json

components/datetime/src/options/components.rs

zbraniecki · 2021-11-04T15:59:55Z

@mildgravitas - that looks good to me! Can you please move all components to use kebab-case? including the ones that are already there now with camel case (like Numeric, Text, Month, Week, TimeZoneName)?

zbraniecki

lgtm! thank you!

nordzilla

LGTM, thank you!

jira-pull-request-webhook · 2021-11-15T18:36:26Z

Notice: the branch changed across the force-push!

components/datetime/src/format/datetime.rs is different
components/datetime/tests/datetime.rs is different
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2021-11-16T15:45:08Z

Notice: the branch changed across the force-push!

components/datetime/src/format/datetime.rs is different
components/datetime/src/provider/date_time.rs is different
components/datetime/src/skeleton/mod.rs is different
components/datetime/tests/datetime.rs is different
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2021-11-18T15:13:00Z

Notice: the branch changed across the force-push!

components/datetime/src/format/datetime.rs is different
components/datetime/src/provider/date_time.rs is different
components/datetime/src/skeleton/helpers.rs is different
components/datetime/src/skeleton/mod.rs is different
components/datetime/tests/datetime.rs is different
provider/testdata/data/json/datetime/gregory_lengths@1/ar-EG.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/ar.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/bn.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/ccp.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/en-001.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/en-ZA.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/en.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/es-AR.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/es.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/fil.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/fr.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/ja.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/ru.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/sr-Cyrl.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/sr-Latn.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/sr.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/th.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/tr.json is no longer changed in the branch
provider/testdata/data/json/datetime/gregory_lengths@1/und.json is no longer changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/ar-EG.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/ar.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/bn.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/ccp.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/en-001.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/en-ZA.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/en.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/es-AR.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/es.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/fil.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/fr.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/ja.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/ru.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/sr-Cyrl.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/sr-Latn.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/sr.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/th.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/tr.json is now changed in the branch
provider/testdata/data/json/datetime/lengths@1/gregory/und.json is now changed in the branch
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2021-11-23T15:41:41Z

Notice: the branch changed across the force-push!

provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

…ed the calendar year).

…th adjustments for it. To do this I've loosened get_best_available_format_pattern() to match on FieldSymbol enums but not their data. From the function's greater/lesser matching this is apparently what the function tried to do all along. Without this Year::NumericWeekOf wouldn't match as CLDR skeletons use 'y' even for patterns with 'Y' This accessorily improves full & long time_h11_h12/time_h23_h24 patterns: the h11_h12/h23_h24 coercion logic matches adjusted patterns against skeletons & previously 'z' was not matched againts 'v' leading to the time zone being dropped. If we don't care to expose the week-of year variants in components::Bag & don't care about coerced time patterns then only adjust_pattern_field_lengths() need be adjusted.

…:components::Year.

…n of datetime::options::components::Year

…bab case for serialization

…an empty pattern if there are no matches.

jira-pull-request-webhook · 2021-12-02T15:52:47Z

Notice: the branch changed across the force-push!

components/datetime/src/error.rs is different
components/datetime/src/fields/symbols.rs is different
components/datetime/src/format/datetime.rs is different
components/datetime/src/options/components.rs is different
components/datetime/src/provider/date_time.rs is different
components/datetime/src/skeleton/helpers.rs is different
components/datetime/src/skeleton/mod.rs is different
components/datetime/tests/datetime.rs is different
components/datetime/tests/fixtures/tests/components-width-differences.json is different
provider/testdata/data/json/datetime/lengths@1/gregory/th.json is different
provider/testdata/data/testdata.postcard is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Manishearth · 2021-12-03T06:10:40Z

@zbraniecki @gregtatum are there any leftover things to do to land this PR? I think we're just missing a final review.

Manishearth · 2021-12-03T06:11:36Z

@mildgravitas btw, we changed how the data is stored, so if you rebase again you probably will want to cargo make download-testdata and then cargo make testdata to generate the new data

mildgravitas · 2021-12-03T10:00:24Z

@Manishearth: Thanks for the heads up. Seems like I'm already top-of-tree from yesterday's rebase onto dc414a8. Running testdata-download && testdata produces no diffs so all good for the time being it seems.

Manishearth · 2021-12-03T10:14:08Z

Ah! I was looking at an older force-push-webhook comment which listed gregory_lengths etc. Yep, your latest push should be fine!

gregtatum

It's looking good to me! Thanks for addressing my comments.

Manishearth · 2021-12-04T10:01:19Z

Going to hit merge since Zibi already gave an "r+ except for minor issues" review above and they seem to have been addressed; I'd rather not let this bitrot again 😄

mildgravitas requested review from gregtatum, nordzilla, zbraniecki and a team as code owners October 25, 2021 15:27

zbraniecki marked this pull request as draft October 25, 2021 18:38

gregtatum removed their request for review October 27, 2021 13:11

mildgravitas force-pushed the year_week_of branch from 5c17d8b to 85130fd Compare October 29, 2021 15:02

mildgravitas marked this pull request as ready for review October 29, 2021 15:40

zbraniecki reviewed Oct 29, 2021

View reviewed changes

mildgravitas force-pushed the year_week_of branch from 9bb7802 to ee46ef0 Compare November 2, 2021 14:29

zbraniecki previously approved these changes Nov 2, 2021

View reviewed changes

mildgravitas dismissed zbraniecki’s stale review via 8889c90 November 2, 2021 16:39

mildgravitas force-pushed the year_week_of branch from 02dd6e0 to eac812c Compare November 2, 2021 17:09

nordzilla requested changes Nov 2, 2021

View reviewed changes

components/datetime/src/skeleton/helpers.rs Outdated Show resolved Hide resolved

components/datetime/benches/fixtures/tests/components.json Outdated Show resolved Hide resolved

components/datetime/src/options/components.rs Outdated Show resolved Hide resolved

zbraniecki previously approved these changes Nov 4, 2021

View reviewed changes

nordzilla self-requested a review November 5, 2021 18:59

nordzilla previously approved these changes Nov 5, 2021

View reviewed changes

sffc requested a review from gregtatum November 8, 2021 15:42

mildgravitas force-pushed the year_week_of branch from 202f1db to 1ad2e0b Compare November 16, 2021 15:45

mildgravitas force-pushed the year_week_of branch from 1ad2e0b to df57b8e Compare November 18, 2021 15:12

mildgravitas requested a review from gregtatum November 19, 2021 15:43

mildgravitas force-pushed the year_week_of branch from df57b8e to a0e798c Compare November 23, 2021 15:41

mildgravitas added 10 commits December 2, 2021 16:18

feat(datetime): Add support for week of year (this previously just us…

4540f5f

…ed the calendar year).

doc(datetime): add examples to the descriptions of datetime::options:…

4a09105

…:components::Year.

fix(datetime): remove serde renames & further expand the documentatio…

d49823a

…n of datetime::options::components::Year

fix(datetime): also update year enum names in benches.

849c839

fix(datetime): swich datetime::options::components::Year & Week to ke…

d3803bb

…bab case for serialization

style(datetime): use is_eq instead of == Ordering:::Equal

adee4d7

fix(datetime): use kebab-case for all options::components enums

e9bca62

tests(datetime): add extra locales to some test cases

fe42ccd

fix(datetime): Return an error in DateTimeFormat::try_new instead of …

8ed516a

…an empty pattern if there are no matches.

mildgravitas force-pushed the year_week_of branch from a0e798c to 8ed516a Compare December 2, 2021 15:52

Manishearth requested review from zbraniecki and nordzilla December 3, 2021 06:10

gregtatum approved these changes Dec 3, 2021

View reviewed changes

Manishearth merged commit a0f78c5 into unicode-org:main Dec 4, 2021

mildgravitas deleted the year_week_of branch December 7, 2021 16:22

Year of week-of-year support #1206

Year of week-of-year support #1206

Conversation

mildgravitas commented Oct 25, 2021 • edited

zbraniecki commented Oct 25, 2021

Manishearth commented Oct 25, 2021

gregtatum commented Oct 25, 2021

Manishearth commented Oct 25, 2021

jira-pull-request-webhook bot commented Oct 29, 2021

mildgravitas commented Oct 29, 2021

zbraniecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki Nov 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

mildgravitas commented Nov 2, 2021

zbraniecki commented Nov 2, 2021

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

mildgravitas commented Nov 2, 2021

nordzilla left a comment

Choose a reason for hiding this comment

zbraniecki commented Nov 4, 2021

zbraniecki left a comment

Choose a reason for hiding this comment

nordzilla left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Nov 15, 2021

jira-pull-request-webhook bot commented Nov 16, 2021

jira-pull-request-webhook bot commented Nov 18, 2021

jira-pull-request-webhook bot commented Nov 23, 2021

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

Manishearth commented Dec 3, 2021

Manishearth commented Dec 3, 2021

mildgravitas commented Dec 3, 2021

Manishearth commented Dec 3, 2021

gregtatum left a comment

Choose a reason for hiding this comment

Manishearth commented Dec 4, 2021

mildgravitas commented Oct 25, 2021 •

edited

zbraniecki Nov 2, 2021 •

edited