Support hc extension key in DateTimeFormat. #135

zbraniecki · 2017-03-17T20:18:19Z

That's the first step to fix #105

zbraniecki · 2017-03-17T20:18:31Z

@caridy - r?

caridy · 2017-03-17T22:16:11Z

will take a look at is next week. also remember that we want to document this process for future similar enhancements.

zbraniecki · 2017-06-27T23:04:39Z

@caridy - ping?

littledan · 2017-07-06T06:39:33Z

spec/datetimeformat.html

@@ -592,7 +600,7 @@
      </p>

      <p>
-        The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this DateTimeFormat object (see <emu-xref href="#sec-properties-of-intl-datetimeformat-instances"></emu-xref>): locale, calendar, numberingSystem, timeZone, hour12, weekday, era, year, month, day, hour, minute, second, and timeZoneName. Properties whose corresponding internal slots have the value *undefined* are not assigned.
+        The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this DateTimeFormat object (see <emu-xref href="#sec-properties-of-intl-datetimeformat-instances"></emu-xref>): locale, calendar, numberingSystem, timeZone, hourCycle, weekday, era, year, month, day, hour, minute, second, and timeZoneName. Properties whose corresponding internal slots have the value *undefined* are not assigned.


Could you add support here for emulating the old hour12 field in resolvedOptions when hourCycle is one of the forms supported by it?

I'm not sure what are you asking here @littledan, can you clarify?

If anyone is using the hour12 field of the resolvedOptions(), this patch makes it go away. That risks web compatibility. For that reason, I'd rather we not merge this patch.

One way to resolve this would be to hold around the hour12 option that the users passed in, and then just pass that back out. I'd suggest, on the other hand, to just use the [[HourCycle]] internal slot that we already have: if the hourCycle is one of the two values that hour12 would provide, then use that in an hour12 property; otherwise, omit the hour12 property.

In my opinion, it's OK to have hour12 in the output even if it wasn't in the input. This is just another instance of the pattern where resolvedOptions has options that the user didn't provide, but were chosen by, e.g., locale selection or matching a pattern.

I've made this suggestion a couple times in the past; I must not have been explaining myself clearly. I'll follow up with spec text to explain the change.

Oh, you're absolutely right! I didn't think of this before. And yes, I agree with always providing the value of hour12 as part of the resolvedOptions. As part of the documentation of the process for this kind of changes, I will document this as well. @zbraniecki can you update the PR?

littledan

Please make sure this does not risk web compatibility before merging.

caridy

hour12 option to always surface as part of the resolvedOptions for web-compat.

zbraniecki · 2017-07-07T16:52:25Z

Updated. @caridy and @littledan - can you re-review please?

caridy

I think that's enough for now, but let's wait for @littledan to look at it since he was the one noticing this.

littledan · 2017-07-15T10:36:05Z

spec/datetimeformat.html

+      </p>
+
+      <p>
+        For web compatibility reasons, if the property hourCycle is set to values *h12* or *h24*, the corresponding property hour12 should be set as well.


This works, but I'm not a huge fan of this wording for a couple minor reasons:

Seems like, suddenly, hourCycle comes last in the enumeration order of the resolvedOptions object. This is extremely minor, but it's a rather unmotivated (only by spec wording constraints) observable effect of the change.

Comments like "for web compatibility reasons" belong in notes, rather than normative text (otherwise it looks a little bit like an "Annex B" normative-optional thing). There are tons and tons of things in all kinds of specs are for web compatibility reasons; probably better to just say what the behavior is.

It would be best to reword the resolvedOptions methods in a more imperative way, as suggested in #124 .

do you want to block landing of this until #124 is resolved?

I'd be happy to apply any wording you can suggest here, but would prefer not to wait for #124.

littledan · 2017-07-15T10:39:33Z

spec/datetimeformat.html

-          1. If _hr12_ is *undefined*, then
-            1. Let _hr12_ be Get(_dataLocaleData_, *"hour12"*).
-          1. Set _dateTimeFormat_.[[Hour12]] to _hr12_.
+          1. Let _hc_ be ? GetOption(_options_, *"hourCycle"*, *"string"*, &laquo; *"h12"*, *"h24"* &raquo;, *undefined*).


Should h11 and h23 be in this list as well? (I thought that was the whole point of the feature.)

No, h11 and h23 can be added later to hourCycle. For now, we do not support them, and I'd prefer to add new features (like support for other hc options) in a separate proposal leaving this just to migrating the option name from a binary, to multioption.

I'd prefer to add hourCycle together with the other possible values. What's the reason to wait? Is the main motivation for this feature to have the hc BCP 47 option?

What's the reason to wait?

Supporting more options requires algorithm changes around hour token formatting.

Ok, I'll work on that, but this will increase the size of this patch substantially and require a re-review.

littledan · 2017-07-15T10:39:45Z

spec/datetimeformat.html

@@ -278,7 +283,7 @@
            1. Let _v_ be the value of _tm_'s field whose name is the Internal Slot column of the matching row.
            1. If _p_ is *"year"* and _v_ ≤ 0, let _v_ be 1 - _v_.
            1. If _p_ is *"month"*, increase _v_ by 1.
-            1. If _p_ is *"hour"* and _dateTimeFormat_.[[Hour12]] is *true*, then
+            1. If _p_ is *"hour"* and _dateTimeFormat_.[[HourCycle]] is *true*, then


Rather than true, do you want to compare it to h11 and h12?

since I don't want to add other hour cycle formats yet, I'll just switch it to 'h12'.

zbraniecki · 2017-07-15T23:14:14Z

@littledan - switched the patch to cover h11 and h23 removing hourNo0. Can you review it please?

littledan · 2017-07-19T14:12:09Z

Oh, I had forgotten about hourNo0--maybe because it's not implemented, tested or documented anywhere but the spec (see #149). Good catch! Because of that, I'd be happy to remove it like this.

Just for completeness, another option for the API surface would be to, instead, reinforce hourNo0. It could be used even when hour12 is false, and then the hc key could be calculated as the combination of the hourNo0 and hour12 options. (The prohibition on hour12 = false and hourNo0 = true could be removed too, to provide h23.) Maybe this could be considered a "cleaner" option, though I don't have a real preference.

LGTM for this, but if @caridy, @jungshik or @srl295 could weigh in on this API vs that potential one.

littledan

I was confused about hourNo0--it was simply part of the data model. And now, that instead comes from the "hourCycle" field on dataLocaleData if not provided by the user--makes sense to me.

Overall, this patch seems great to me; sorry for my trouble in understanding it.

littledan · 2017-07-19T16:27:04Z

spec/datetimeformat.html

@@ -622,8 +640,7 @@
      <li>[[NumberingSystem]] is a String value with the "type" given in Unicode Technical Standard 35 for the numbering system used for formatting.</li>
      <li>[[TimeZone]] is a String value with the IANA time zone name of the time zone used for formatting.</li>
      <li>[[Weekday]], [[Era]], [[Year]], [[Month]], [[Day]], [[Hour]], [[Minute]], [[Second]], [[TimeZoneName]] are each either *undefined*, indicating that the component is not used for formatting, or one of the String values given in <emu-xref href="#table-datetimeformat-components"></emu-xref>, indicating how the component should be presented in the formatted output.</li>
-      <li>[[Hour12]] is a Boolean value indicating whether 12-hour format (*true*) or 24-hour format (*false*) should be used. It is only used when [[Hour]] is not *undefined*.</li>
-      <li>[[HourNo0]] is a Boolean value indicating whether hours from 1 to 12 (*true*) or from 0 to 11 (*false*) should be used. It is only used when [[Hour12]] has the value *true*.</li>
+      <li>[[HourCycle]] is a String value indicating whether 12-hour format (*"h11"*, *"h12"*) or 24-hour format (*"h23"*, *"h24"*) should be used. It is only used when [[Hour]] is not *undefined*.</li>


Should this description explain the difference between the 11/23 and 12/24 values?

Good fix, seems ready to land.

zbraniecki · 2017-07-20T05:13:33Z

I think it's all ready to land then :)

caridy · 2017-07-20T15:18:38Z

@zbraniecki can you help with the tests needed for this?

littledan · 2017-07-20T16:33:22Z

Sorry that I didn't state this earlier, but I think that in addition to the patch looking good, it's nice if we have a couple things in place before actually merging:

test262 tests (this is a theoretical process requirement that the committee adopted, but it's not being followed consistently in the main spec either)
implementation experience in one engine (this is not generally a requirement, but I'd prefer it for non-trivial things so we can know that it's a decent plan, and that it's likely to become web reality)

I guess it's already merged, but if we could keep these requirements in mind for future normative changes, it'd be nice.

zbraniecki · 2017-08-01T06:12:45Z

I'm working on the implementation for SpiderMonkey in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1386146 and will add test262 tests as well in the process.

Support hc extension key in DateTimeFormat.

486e3c0

caridy added the enhancement label Mar 17, 2017

caridy self-requested a review March 17, 2017 22:15

littledan requested changes Jul 6, 2017

View reviewed changes

caridy approved these changes Jul 7, 2017

View reviewed changes

caridy added the needs tests label Jul 7, 2017

littledan requested changes Jul 7, 2017

View reviewed changes

caridy requested changes Jul 7, 2017

View reviewed changes

Add hour12 to resolvedOptions

d4392b6

caridy approved these changes Jul 14, 2017

View reviewed changes

littledan reviewed Jul 15, 2017

View reviewed changes

Zibi Braniecki added 4 commits July 15, 2017 09:35

Apply reviewers feedback

57e8af2

Add h11 and h23

c456ed4

Use local field hourCycle as well

654f302

Fix annex

78f29d3

littledan mentioned this pull request Jul 19, 2017

hourNo0 does not seem be shipped in any browser #149

Closed

littledan approved these changes Jul 19, 2017

View reviewed changes

Fix annex

29d6932

zbraniecki force-pushed the hour-cycle branch from 4ca6216 to 29d6932 Compare July 20, 2017 05:11

caridy merged commit bb18b48 into tc39:master Jul 20, 2017

zbraniecki mentioned this pull request Oct 6, 2017

Intl.DateTimeFormat hourCycle tests tc39/test262#1265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hc extension key in DateTimeFormat. #135

Support hc extension key in DateTimeFormat. #135

zbraniecki commented Mar 17, 2017

zbraniecki commented Mar 17, 2017

caridy commented Mar 17, 2017

zbraniecki commented Jun 27, 2017

littledan Jul 6, 2017

caridy Jul 7, 2017

littledan Jul 7, 2017

caridy Jul 7, 2017 •

edited

Loading

littledan left a comment

caridy left a comment

zbraniecki commented Jul 7, 2017

caridy left a comment

littledan Jul 15, 2017

zbraniecki Jul 15, 2017

littledan Jul 15, 2017

zbraniecki Jul 15, 2017

littledan Jul 15, 2017

zbraniecki Jul 15, 2017

littledan Jul 15, 2017

zbraniecki Jul 15, 2017

zbraniecki commented Jul 15, 2017

littledan commented Jul 19, 2017 •

edited

Loading

littledan left a comment

littledan Jul 19, 2017

littledan Jul 20, 2017

zbraniecki commented Jul 20, 2017

caridy commented Jul 20, 2017

littledan commented Jul 20, 2017

zbraniecki commented Aug 1, 2017

Support hc extension key in DateTimeFormat. #135

Support hc extension key in DateTimeFormat. #135

Conversation

zbraniecki commented Mar 17, 2017

zbraniecki commented Mar 17, 2017

caridy commented Mar 17, 2017

zbraniecki commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

littledan left a comment

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

zbraniecki commented Jul 7, 2017

caridy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki commented Jul 15, 2017

littledan commented Jul 19, 2017 • edited Loading

littledan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki commented Jul 20, 2017

caridy commented Jul 20, 2017

littledan commented Jul 20, 2017

zbraniecki commented Aug 1, 2017

caridy Jul 7, 2017 •

edited

Loading

littledan commented Jul 19, 2017 •

edited

Loading