Add support for width-aware noon and midnight options for DayPeriod #444

nordzilla · 2021-01-12T01:34:27Z

Fixes #435

This patch adds functionality and tests for width-aware DayPeriod patterns for noon and midnight.

https://unicode.org/reports/tr35/tr35-dates.html#dfst-period

According to the spec, when formatting with this pattern:

The noon options are used from 12:00:00 before 12:01:00
The midnight options are used from 00:00:00 before 00:01:00
If the locale does not support noon and/or midnight the appropriate AmPm option will be used as a fallback.
All other times will continue to use AmPm as expected.

There is no public functionality in icu4x to create a DateTimeFormat with a custom pattern. The function that handles formatting with a pattern, write_pattern(), lives in the format module, which is private. In order for tests to have access to the write_pattern() function, all testing must live within the format module itself, so this patch restructures the previous format.rs module into format/mod.rs and adds a format/tests directory where the relevant testing lives.

nordzilla · 2021-01-12T01:36:07Z

components/datetime/src/format/mod.rs

-            }
-        }
-    }
-}


This test moved to the newtests/mod.rs file.

Is this a conventional place to save tests? @zbraniecki @Manishearth

Why not /components/datetime/tests/dayperiod.rs ?

Given that you are writing these tests as parameterized tests with fixtures, I definitely think that they belong as an integration test, not hidden in a test directory in the library.

(sorry for the triple-comment)

I would have liked for it to be an integration test, but as I noted in the description of the PR, there is no public functionality to format dates with a custom pattern, so it has to live within the privateformat module. This seemed, to me, to be the best compromise, but I'm open to other suggestions.

For unit tests it's fine to create a tests module. I personally don't feel a need to prefer integration tests over unit tests, though it is definitely easier to find integration tests.

@sffc So I looked into this a bit more this morning, specifically into how I might use StructProvider.

Even if I used StructProvider to get a custom payload, it seems that both write_pattern() and DateTimeFormat must take a DatesV1 struct specifically as their data.

Unless there is another way to format the datetime using the custom provider, I'm not seeing any viable options to format with a custom pattern publicly. That being said, I'm still trying to build an overall familiarity with the code base, so I certainly could have missed a key insight here.

Right. Create a DatesV1, either using Default::default() or by loading a stock locale from test data, then modify a field like patterns.time.full with your desired pattern.

I like the idea of data-driven tests being able to overwrite locale data for that testing. @zbraniecki thoughts?

Okay, so I've now moved this test back to the file where it existed previously, and I moved the new tests to the components/datetime/tests folder to be integration tests. I really like this a lot more.

But it feels somewhat hacky to me to grab the CLDR data for the locale, and then just overwrite the long patterns for each relevant case in my test data. Is this really intended functionality? I had previously overlooked that every member in DatesV1 is public.

I had a conversation with @zbraniecki about how custom patterns are often misused in practice. My understanding is that this is why we don't really have a public functionality to directly do something like DateTimeFormat::try_format(&date_time, &pattern), and that we prefer to have people use only the options bags.

It just seems strange to me to have this preference to funnel people through pre-existing options, but still allow "backdoors" for custom patterns by letting people directly mutate the DatesV1 data directly. I'm almost of the opinion that, if the functionality exists anyway, why not provide something likeDateTimeFormat::try_format(&date_time, &pattern)?

I suppose I just want to gain clarity around the design decisions behind not having a custom format with pattern function, and for making all of the DatesV1 members public, since I wasn't involved in the project at the time when these were conceived or implemented.

In the interest of not having duplicated conversations, if there are links to relevant pre-existing conversations that would be helpful to me.

In the interest of not having duplicated conversations, if there are links to relevant pre-existing conversations that would be helpful to me.

We've had conversations about this before, but I think they weren't written down.

The fields in all the data structs are public by design. The data struct is designed to be the place where you can "overlay" stock data with custom overrides.

That being said, I agree that in this particular case, it would probably be cleaner to have a low level pattern API. We can revisit this test when we have such API.

jira-pull-request-webhook · 2021-01-12T01:40:13Z

Notice: the branch changed across the force-push!

components/datetime/src/format/tests/mod.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

jira-pull-request-webhook · 2021-01-12T01:41:40Z

Notice: the branch changed across the force-push!

components/datetime/src/format/tests/patterns/tests/dayperiods.json is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

codecov-io · 2021-01-12T01:44:59Z

Codecov Report

Merging #444 (fc237db) into master (d2f47f7) will increase coverage by 0.20%.
The diff coverage is 90.12%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   75.28%   75.49%   +0.20%     
==========================================
  Files         113      113              
  Lines        6058     6116      +58     
==========================================
+ Hits         4561     4617      +56     
- Misses       1497     1499       +2

Impacted Files	Coverage Δ
components/datetime/src/provider/mod.rs	`0.00% <0.00%> (ø)`
components/provider_cldr/src/transform/dates.rs	`69.02% <ø> (ø)`
components/datetime/src/pattern/mod.rs	`86.79% <88.00%> (+2.41%)`	⬆️
components/datetime/src/pattern/parser.rs	`96.47% <97.61%> (+0.05%)`	⬆️
components/datetime/src/fields/symbols.rs	`77.31% <100.00%> (+0.23%)`	⬆️
components/datetime/src/format.rs	`100.00% <100.00%> (ø)`
components/datetime/src/provider/helpers.rs	`68.75% <100.00%> (+1.72%)`	⬆️
components/datetime/src/date.rs	`59.34% <0.00%> (+2.19%)`	⬆️

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 d2f47f7...fc237db. Read the comment docs.

components/datetime/src/provider.rs

coveralls · 2021-01-12T01:46:00Z

Pull Request Test Coverage Report for Build d48ff169ff786e1a24ae8e08ab4a6c8c62092f22-PR-444

73 of 81 (90.12%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 74.783%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/datetime/src/pattern/parser.rs	41	42	97.62%
components/datetime/src/pattern/mod.rs	22	25	88.0%
components/datetime/src/provider/mod.rs	0	4	0.0%

Totals
Change from base Build d2f47f76a82b64a1204bec2a81130fcb70fcea06:	0.2%
Covered Lines:	4733
Relevant Lines:	6329

💛 - Coveralls

zbraniecki

lgtm!

components/provider_cldr/src/transform/dates.rs

sffc

(setting review bit)

jira-pull-request-webhook · 2021-01-14T22:14:58Z

Notice: the branch changed across the force-push!

components/datetime/src/lib.rs is no longer changed in the branch
components/datetime/src/options/components.rs is no longer changed in the branch
components/datetime/src/options/style.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

components/provider/src/structs/dates.rs

jira-pull-request-webhook · 2021-01-15T23:03:05Z

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

~ Your Friendly Jira-GitHub PR Checker Bot

components/datetime/src/provider.rs

zbraniecki

lgtm!

nordzilla · 2021-01-21T22:45:45Z

I just want to clarify everything that we have discussed formally in a comment.

So the goal is to add two new sets of functionality to this PR:

Modify the DataProvider to generate special patterns, as @sffc noted (pseudo-syntax)

if (m == 0) {
  if (h == 0) {
    "'midnight'"
  } else if (h == 12) {
    "'noon'"
  } else {
    "h a"
  }
} else {
  "h:mm a"
}

As DateTimeFormat is constructed, when tokenizing the pattern, determine which dayperiod fields are needed based on how specific the display is.

Is this correct?

I think that number 2) makes sense, I am still a bit unclear still about number 1) because the DataProvider isn't necessarily aware of any specific datetime. What does m == 0 mean in the context of only the DataProvider?

It's the DateTimeFormat that puts this all together. I would think all of this logic should live in DateTimeFormat.

sffc · 2021-01-21T22:49:02Z

The pseudo-syntax I posted above is something we should do later in #380. For now we just need to do what you posted in #444 (comment), which is a bit simpler.

nordzilla · 2021-01-23T01:20:31Z

As a first pass to achieve the functionality described in #444 (comment), I have naively added the capability for a Pattern to retrieve the maximum time granularity contained inside the pattern.

Currently, the maximum granularity is computed every time the pattern is written. This is obviously going to be slower than computing the maximum granularity once on creation and caching the value, but it was one of the simplest approaches that touched the least amount of existing code in order to get new tests written for the functionality.

I am curious to see if and how much this regresses the benchmarks. I suspect that I will likely add one more commit to compute and cache this value.

components/datetime/src/format/mod.rs

components/datetime/src/pattern/mod.rs

components/datetime/src/provider.rs

jira-pull-request-webhook · 2021-01-25T20:46:40Z

Notice: the branch changed across the force-push!

components/datetime/src/format/mod.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

- Restructures `format` module to use directory structure instead of single file. - Adds a `format/tests` directory. - Moves existing local tests from `format.rs` to `format/tests/mod.rs` - Adds JSON test data for `DayPeriod` patterns. - Adds JSON-serializable structs for testing formatting patterns. - Adds test cases for `DayPeriod` patterns. - Adds parsing test cases for the `b` `DayPeriod` pattern.

- 24:00 will not be a valid time once unicode-org#355 is fixed. - Removes logic than handles 24:00 for now.

- Converts `DayPeriod` pattern tests to be integration tests. - Tests no longer direclty use the private `write_pattern()`. - Tests now mutate the `DatesV1` struct to the desired pattern, using `DateTimeFormat` to format the custom patterns.

jira-pull-request-webhook · 2021-01-29T21:44:13Z

Notice: the branch changed across the force-push!

components/datetime/src/format.rs is different
components/datetime/src/provider.rs is no longer changed in the branch
components/datetime/src/provider/helpers.rs is now changed in the branch
components/datetime/src/provider/mod.rs is now changed in the branch
components/datetime/tests/datetime.rs is different
components/provider_cldr/src/transform/dates.rs is different
components/provider/src/structs/dates.rs is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

nordzilla · 2021-01-29T21:47:33Z

All right @sffc I rebased on top of master after you landed #451.

This should be ready for one final look (no changes since last time), and then able to merge with no conflicts.

nordzilla · 2021-01-29T21:56:03Z

Uh-oh, spoke too soon. Looks like the CI is having an issue with the optional serde serialization, even though it works for me locally.

It probably has something to do with you making serde optional. I will look into this.

- Rewrites the `symbols!()` macro as a token tree muncher. - For `Option` members, adds serde attributes to skip serializing if none. - Otherwise, includes them in the seralization.

- Regenerates the test data now that `noon` and `midnight` are skipped if not present. - Previously `noon` and `midnight` would show up as `null`.

- Moves a few expressions in the dayperiod patterns test to outer loops.

- Adds capability for a pattern to compute its most granular time. - e.g. `h:mm:ss` is `Seconds`. - e.g. `h` is `Hours`. - e.g. `E, dd/MM/y` is `None`. - Patterns containing `b` the `NoonMidnight` pattern item will now display noon or midnight only if the displayed time falls on the hour. - This means that `12:00:00` is always noon-compatible. - However, `12:05:15` is noon-compatible only if the display pattern does not contain the minutes or seconds.

- Time granularity functionality is no longer associated with Pattern or PatternItem. It is now local to the format module alone as standalone functions.

- Format no longer needs to be a directory.

- Makes TimeGranularity private instead of public.

- Only calculates the time granularity if the `DayPeriod` is `NoonMidnight`.

- Converts `Pattern` from a tuple struct to a traditional struct. - Adds a new data member `time_granularity` to `Pattern`. - `time_granularity` is a lazily initialized, interrior-mutable cached value. - Makes `Pattern`'s data members private. - The cached `time_granularity` is dependent on the `Pattern`'s `items`. It is no longer safe to allow `items` to be publicly accessible, because mutating `items` must invalidate the cached granularity. - Adds new method `items()` to `Pattern` to return a slice of its items. - Implement `From<Vec<PatternItem>` for `Pattern` - This is out of convenience in many places where tuple-struct syntax was used previously.

- Pattern::from_iter now uses Pattern::from::<Vec<_>>

- `Pattern`'s time granularity is no longer lazily evaluated. - It is instead evaulated on construction.

jira-pull-request-webhook · 2021-01-29T22:04:35Z

Notice: the branch changed across the force-push!

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

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

nordzilla · 2021-01-29T22:27:01Z

@sffc all fixed. The CI failure was due to me not handling optional serde in the macro rewrite. Should be good to go now.

components/datetime/src/pattern/mod.rs

- filter_map is more specialized, and arguably more readable.

zbraniecki

lgtm!

nordzilla requested review from sffc and zbraniecki as code owners January 12, 2021 01:34

nordzilla commented Jan 12, 2021

View reviewed changes

nordzilla force-pushed the day-period-noon-midnight branch from 572aff5 to dfebcfd Compare January 12, 2021 01:40

nordzilla force-pushed the day-period-noon-midnight branch from dfebcfd to ff5df4d Compare January 12, 2021 01:41

nordzilla commented Jan 12, 2021

View reviewed changes

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

nordzilla changed the title ~~Day period noon midnight~~ Add support for width-aware noon and midnight options for DayPeriod Jan 12, 2021

zbraniecki previously approved these changes Jan 12, 2021

View reviewed changes

components/provider_cldr/src/transform/dates.rs Show resolved Hide resolved

sffc requested changes Jan 12, 2021

View reviewed changes

nordzilla dismissed zbraniecki’s stale review via ddd7588 January 12, 2021 20:37

nordzilla force-pushed the day-period-noon-midnight branch from 2e710b3 to 3ed730e Compare January 14, 2021 22:14

sffc reviewed Jan 15, 2021

View reviewed changes

components/provider/src/structs/dates.rs Outdated Show resolved Hide resolved

nordzilla force-pushed the day-period-noon-midnight branch from ac00238 to efe4f59 Compare January 15, 2021 23:03

zbraniecki reviewed Jan 15, 2021

View reviewed changes

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

zbraniecki previously approved these changes Jan 15, 2021

View reviewed changes

nordzilla requested a review from sffc January 19, 2021 20:08

nordzilla dismissed zbraniecki’s stale review via 3ce576c January 20, 2021 20:18

zbraniecki reviewed Jan 25, 2021

View reviewed changes

nordzilla force-pushed the day-period-noon-midnight branch from 15c6b89 to 770936d Compare January 25, 2021 20:46

nordzilla force-pushed the day-period-noon-midnight branch from 7c08500 to ba149fd Compare January 27, 2021 21:45

nordzilla added 3 commits January 29, 2021 13:25

Remove logic that handles 24:00

263c88a

- 24:00 will not be a valid time once unicode-org#355 is fixed. - Removes logic than handles 24:00 for now.

nordzilla dismissed sffc’s stale review via 6c87afd January 29, 2021 21:44

nordzilla force-pushed the day-period-noon-midnight branch from bbfe8a5 to 6c87afd Compare January 29, 2021 21:44

nordzilla added 11 commits January 29, 2021 14:03

Rewrite symbols!() macro to support serde_none seralization for Options

69f2825

- Rewrites the `symbols!()` macro as a token tree muncher. - For `Option` members, adds serde attributes to skip serializing if none. - Otherwise, includes them in the seralization.

Regenerate test data with optional serializtion for dayperiods

971c9ad

- Regenerates the test data now that `noon` and `midnight` are skipped if not present. - Previously `noon` and `midnight` would show up as `null`.

Minor Test Cleanup

93cbe2e

- Moves a few expressions in the dayperiod patterns test to outer loops.

Move time granularity functions to format-local helper functions.

c96ce86

- Time granularity functionality is no longer associated with Pattern or PatternItem. It is now local to the format module alone as standalone functions.

Move format/mod.rs to format.rs

41dfafc

- Format no longer needs to be a directory.

Fix access specifiers

ca7f60c

- Makes TimeGranularity private instead of public.

Add minor DayPeriod formatting optimization.

5bf3875

- Only calculates the time granularity if the `DayPeriod` is `NoonMidnight`.

Clean up Pattern::from_iter

043f96e

- Pattern::from_iter now uses Pattern::from::<Vec<_>>

Eagerly evaluate Pattern time granularity

4d1ca3b

- `Pattern`'s time granularity is no longer lazily evaluated. - It is instead evaulated on construction.

nordzilla force-pushed the day-period-noon-midnight branch from 6c87afd to 4d1ca3b Compare January 29, 2021 22:04

nordzilla requested a review from sffc January 29, 2021 22:48

sffc previously approved these changes Jan 29, 2021

View reviewed changes

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

Use filter_map instead of flat_map

fc237db

- filter_map is more specialized, and arguably more readable.

nordzilla dismissed sffc’s stale review via fc237db January 29, 2021 23:45

sffc approved these changes Jan 29, 2021

View reviewed changes

zbraniecki approved these changes Jan 29, 2021

View reviewed changes

nordzilla merged commit 1d14a5d into unicode-org:master Jan 30, 2021

zbraniecki mentioned this pull request Apr 14, 2021

ICU4X 0.2 #239

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for width-aware noon and midnight options for DayPeriod #444

Add support for width-aware noon and midnight options for DayPeriod #444

nordzilla commented Jan 12, 2021

nordzilla Jan 12, 2021 •

edited

Loading

sffc Jan 12, 2021

sffc Jan 12, 2021

sffc Jan 12, 2021

nordzilla Jan 12, 2021

Manishearth Jan 13, 2021 •

edited

Loading

nordzilla Jan 13, 2021

sffc Jan 13, 2021

nordzilla Jan 14, 2021 •

edited

Loading

sffc Jan 15, 2021

jira-pull-request-webhook bot commented Jan 12, 2021

jira-pull-request-webhook bot commented Jan 12, 2021

codecov-io commented Jan 12, 2021 •

edited

Loading

coveralls commented Jan 12, 2021 •

edited

Loading

zbraniecki left a comment

sffc left a comment

jira-pull-request-webhook bot commented Jan 14, 2021

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

zbraniecki left a comment

nordzilla commented Jan 21, 2021

sffc commented Jan 21, 2021

nordzilla commented Jan 23, 2021 •

edited

Loading

jira-pull-request-webhook bot commented Jan 25, 2021

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

nordzilla commented Jan 29, 2021

nordzilla commented Jan 29, 2021

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

nordzilla commented Jan 29, 2021

zbraniecki left a comment

Add support for width-aware noon and midnight options for DayPeriod #444

Add support for width-aware noon and midnight options for DayPeriod #444

Conversation

nordzilla commented Jan 12, 2021

nordzilla Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

sffc Jan 12, 2021

Choose a reason for hiding this comment

sffc Jan 12, 2021

Choose a reason for hiding this comment

sffc Jan 12, 2021

Choose a reason for hiding this comment

nordzilla Jan 12, 2021

Choose a reason for hiding this comment

Manishearth Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

nordzilla Jan 13, 2021

Choose a reason for hiding this comment

sffc Jan 13, 2021

Choose a reason for hiding this comment

nordzilla Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

sffc Jan 15, 2021

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jan 12, 2021

jira-pull-request-webhook bot commented Jan 12, 2021

codecov-io commented Jan 12, 2021 • edited Loading

Codecov Report

coveralls commented Jan 12, 2021 • edited Loading

Pull Request Test Coverage Report for Build d48ff169ff786e1a24ae8e08ab4a6c8c62092f22-PR-444

💛 - Coveralls

zbraniecki left a comment

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Jan 14, 2021

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

zbraniecki left a comment

Choose a reason for hiding this comment

nordzilla commented Jan 21, 2021

sffc commented Jan 21, 2021

nordzilla commented Jan 23, 2021 • edited Loading

jira-pull-request-webhook bot commented Jan 25, 2021

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

nordzilla commented Jan 29, 2021

nordzilla commented Jan 29, 2021

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

nordzilla commented Jan 29, 2021

zbraniecki left a comment

Choose a reason for hiding this comment

nordzilla Jan 12, 2021 •

edited

Loading

Manishearth Jan 13, 2021 •

edited

Loading

nordzilla Jan 14, 2021 •

edited

Loading

codecov-io commented Jan 12, 2021 •

edited

Loading

coveralls commented Jan 12, 2021 •

edited

Loading

nordzilla commented Jan 23, 2021 •

edited

Loading