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

[datetime] Add support for width-aware noon and midnight DayPeriods #435

Closed
nordzilla opened this issue Jan 5, 2021 · 8 comments · Fixed by #444
Closed

[datetime] Add support for width-aware noon and midnight DayPeriods #435

nordzilla opened this issue Jan 5, 2021 · 8 comments · Fixed by #444
Assignees
Labels
C-datetime Component: datetime, calendars, time zones T-core Type: Required functionality
Milestone

Comments

@nordzilla
Copy link
Member

This fits into ICU4X 0.2 roadmap (#239) under DateTimeFormat.

@nordzilla
Copy link
Member Author

I am planning to work on this.

@zbraniecki zbraniecki added this to the ICU4X 0.2 milestone Jan 5, 2021
@zbraniecki
Copy link
Member

Hi Erik! Thank you for picking it up!

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

We currently support a very very basic am/pm in https://github.com/unicode-org/icu4x/blob/master/components/provider/src/structs/dates.rs#L102 and use it in https://github.com/unicode-org/icu4x/blob/master/components/datetime/src/provider.rs#L195-L199

@sffc suggested that it may be worth separating our flexible periods since they are quite a bit more complex with their own rules and parsers etc.

So, what we suggest to focus on for 0.2 is to get:

  • width aware a field
  • width aware b field

This will require a little bit of calculations for noon/midnight, but not that much, and mostly it's about adding the data to the right places.

One design decision you may want to consider is whether it's worth separating day periods out to a separate data provider key in ICU4X data. The reason here is that constructor knows if day periods will be needed, so for all dates which don't use them we could save some memory and bandwidth by not loading them.

@sffc, @mihnita - is that a good model to use? I know you talked about potentially adding methods to change display in place, but my hope is that we can still assume that if the constructor identified that dayperiods are not needed, instance mutations of the formatter cannot make it necessary.

@sffc
Copy link
Member

sffc commented Jan 6, 2021

I talk about day periods a bit in #355. In my mind, the key question is, can day periods be deterministically computed from the time of day? From my discussions with Mark, Mihai, and others, it seems the answer is YES for a and b and NO for B. Therefore, since we are punting on B, the nature of this issue is just looking for a and b in pattern strings and returning the deterministically computed day period based on the time of day.

Can we keep the discussion about whether to split day periods into their own data key in #257?

@zbraniecki
Copy link
Member

Can we keep the discussion about whether to split day periods into their own data key in #257?

Yep!

@nordzilla do you have all necessary information to tackle this now?

@nordzilla
Copy link
Member Author

@zbraniecki

Yes, I believe so! I'll certainly have questions, but I'm starting work on it.

@nordzilla
Copy link
Member Author

nordzilla commented Jan 7, 2021

I've run into my first roadblock where I have some questions:

After adding the relevant symbols, I've been trying to generate new test data that includes noon and midnight, but I am getting an error that af/ca-gregorian.json doesn't actually have noon.

When I try to generate the test data via cargo gen-testdata I get:

Error: Failed to load resource: JSON parse error: missing field `noon` at line 233 column 17: "/.../main/af/ca-gregorian.json"

I'm going to pick up tomorrow with looking into if/how we can support optional symbols. Presumably if the noon isn't included in a user's CLDR, we should just fall back to using PM?

@zbraniecki @sffc

I'm still very new to this code base, and entirely new to internationalization. If you have any insights or pointers, they would be much appreciated.

@sffc
Copy link
Member

sffc commented Jan 7, 2021

Start with the spec:

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

All locales must support am/pm, but not all support noon or midnight; they are only supported if they meet the above definitions. For example, German has no unique term that means exactly 12:00 noon; the closest is Mittag, but that can extend before or after 12 noon.

This data file is probably going to be useful; sorry for not flagging it sooner:

https://github.com/unicode-org/cldr-json/blob/master/cldr-json/cldr-core/supplemental/dayPeriods.json

@sffc
Copy link
Member

sffc commented Jan 7, 2021

The data file linked above (dayPeriods.json) will be useful for B. However, I think it's probably safe to hard-code the rules for a and b, although maybe do a sanity check and add a test to assert that you can do that safely.

Basically, noon falls back to pm if the noon symbol is unavailable in the locale's data file, and midnight falls back to am if the midnight symbol is not available.

@nordzilla nordzilla changed the title [datetime] Add support for DayPeriods [datetime] Add support for width-aware noon and midnight DayPeriods Jan 12, 2021
@sffc sffc added C-datetime Component: datetime, calendars, time zones T-core Type: Required functionality labels Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-datetime Component: datetime, calendars, time zones T-core Type: Required functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants