tc39 / proposal-intl-datetime-style

dateStyle and timeStyle options for DateTimeFormat
https://tc39.github.io/proposal-intl-datetime-style/
28 stars 9 forks source link

Improve handling of date/time style field widths. #48

Closed zbraniecki closed 4 years ago

zbraniecki commented 4 years ago

Fixes #45

zbraniecki commented 4 years ago

@littledan @anba I know it's pretty informal, but I hope we're ok not having to write an algo for getting the fields from the pattern. What do you think?

anba commented 4 years ago

This only sets the internal slots to undefined, whereas we need to set them to, e.g. "2-digit" or "numeric", etc.

I know it's pretty informal, but I hope we're ok not having to write an algo for getting the fields from the pattern.

As mentioned in https://github.com/tc39/proposal-intl-datetime-style/issues/45#issuecomment-640049033, the overall structure for [[DateFormat]], [[TimeFormat]], and [[DateTimeFormat]] needs to be modified first. As soon as these records contain fields for the field widths, it should be easier to modify InitializeDateTimeFormat.

zbraniecki commented 4 years ago

@anba - does this look like the right direction?

anba commented 4 years ago

I was thinking to try something along the lines of https://github.com/anba/proposal-intl-datetime-style/commits/field-widths:

  1. https://github.com/anba/proposal-intl-datetime-style/commit/64f62aad2fa47e4edcdeca924a2b6d075d38ea10 modifies the internal slot description to include field widths.
  2. https://github.com/anba/proposal-intl-datetime-style/commit/c72c18d83d42381311d89a60f2adc36c99afe288 changes DateTimeStylePattern (renamed to DateTimeStyleFormat) to return a format record. (The record has the same structure as the one returned from BasicFormatMatcher and BestFitFormatMatcher). The Intl.DateTimeFormat instance's internal slots are then initialised from that record. The [[pattern]] and [[pattern12]] selection now also happens in InitializeDateTimeFormat. This is in preparation for the next commits.
  3. https://github.com/anba/proposal-intl-datetime-style/commit/80ee9a6712eb578034a2de24e238513d2d3cd791 merges steps which now happen in both arms of the if-statement.
  4. https://github.com/anba/proposal-intl-datetime-style/commit/3c1213e4d10fef9b6d7936ca74eeb225bbd67292 removes an explicit step to initialise [[HourCycle]], which is now no longer needed.
  5. https://github.com/anba/proposal-intl-datetime-style/commit/fd3357ae5263b3c8e18270d804da6c615b42d61e moves the hour-cycle computation back into the same place where it happens in ECMA-402. This is possible, because DateTimeStyleFormat was changed to return a record.
  6. https://github.com/anba/proposal-intl-datetime-style/commit/1e3398616770fe124426612ee8484d92ad6cfe36 modifies resolvedOptions to not output date-time fields when dateStyle or timeStyle is present.
  7. https://github.com/anba/proposal-intl-datetime-style/commit/0e1e6aa6349d92182dccb2f66d359f34ed4eb19b removes another modification from ECMA-402, which is also no longer necessary.
zbraniecki commented 4 years ago

I was thinking to try something along the lines of https://github.com/anba/proposal-intl-datetime-style/commits/field-widths:

Mhm, that seems reasonable. I started moving in that direction, but basically stopped on a variant of your (2) when I realized that that can be folded into DateTimeStylePattern function and wanted to get your feedback :)

2. [anba@c72c18d](https://github.com/anba/proposal-intl-datetime-style/commit/c72c18d83d42381311d89a60f2adc36c99afe288) changes `DateTimeStylePattern` (renamed to `DateTimeStyleFormat`) to return a format record. (The record has the same structure as the one returned from `BasicFormatMatcher` and `BestFitFormatMatcher`). The `Intl.DateTimeFormat` instance's internal slots are then initialised from that record. The [[pattern]] and [[pattern12]] selection now also happens in `InitializeDateTimeFormat`. This is in preparation for the next commits.
3. [anba@80ee9a6](https://github.com/anba/proposal-intl-datetime-style/commit/80ee9a6712eb578034a2de24e238513d2d3cd791) merges steps which now happen in both arms of the if-statement.

I like that a lot.

6. [anba@1e33986](https://github.com/anba/proposal-intl-datetime-style/commit/1e3398616770fe124426612ee8484d92ad6cfe36) modifies `resolvedOptions` to not output date-time fields when `dateStyle` or `timeStyle` is present.

I'm wondering if it would be more readable to separate those two loops - for dateStyle/timeStyle vs for fields. But maybe since we're discussing what to do with it in #49 it's ok to keep it like this?

7. [anba@0e1e6aa](https://github.com/anba/proposal-intl-datetime-style/commit/0e1e6aa6349d92182dccb2f66d359f34ed4eb19b) removes another modification from ECMA-402, which is also no longer necessary.

Yay!

@anba - thank you so much for this patchset. It's clear and well thought through and I like how it even brings the PR closer to the ECMA402 spec than we were before! Would you be open to opening it as a PR? I don't think it makes sense for me to migrate it to my PR, since yours is much more elegant.

anba commented 4 years ago

Would you be open to opening it as a PR?

I'll wait for #50 to avoid any merge conflicts, okay?

zbraniecki commented 4 years ago

Merged #50

littledan commented 4 years ago

This change broadly makes sense, but ultimately I like how @anba 's series phrases it. So I'm happy for that other PR to land once it's ready. Thanks for all this work.

zbraniecki commented 4 years ago

I agree. I'm going to close this PR in favor of @anba 's one.