-
Notifications
You must be signed in to change notification settings - Fork 106
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
[4th Edition] DateTimeFormat.formatToParts() #64
Conversation
77d7832
to
36ea4a4
Compare
|
||
<p> | ||
When the FormatDateTime abstract operation is called with arguments _dateTimeFormat_ (which must be an object initialized as a DateTimeFormat) and _x_ (which must be a Number value), it returns a String value representing _x_ (interpreted as a time value as specified in ES2015, 20.3.1.1) according to the effective locale and the formatting options of _dateTimeFormat_. This abstract operation functions as follows: | ||
A DateTime formatToParts function is an anonymous function that optionally takes an argument _date_. It performs the following steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a DateTime formatToParts function is called with optional argument date, the following steps are taken:
Also, we should resolve the conflicts with master before producing a new HTML for the formal review from @littledan |
1. Let _fv_ be an implementation and locale dependent String value representing "post meridiem"; | ||
1. Else, | ||
1. Let _fv_ be an implementation and locale dependent String value representing "ante meridiem". | ||
1. Add new part record { [[type]]: "dayperiod", [[value]]: _fv_ } as a new element of the list _result_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this part be named to "dayPeriod" for consistency with other compound names (e.g. "timeZoneName")?
Plus, |
|
||
<emu-clause id="alg-CreateDateTimeParts"> | ||
|
||
<h1>CreateDateTimeParts(dateTimeFormat, x)</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5b3c21a
to
593b9a4
Compare
@caridy - rebased on top of master and applied stas's feedback |
<p> | ||
The *length* property of a DateTime format function is 1. | ||
</p> | ||
</emu-clause> | ||
|
||
<emu-clause id="sec-formatdatetime" aoid="FormatDateTime"> | ||
<h1>FormatDateTime (dateTimeFormat, x)</h1> | ||
<emu-clause id="sec-formatdatetimetoparts" aoid="PartitionDateTimePattern"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the id
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thnx! fixed
68fa6ae
to
ca27e01
Compare
1. Let _O_ be ObjectCreate(%ObjectPrototype%). | ||
1. CreateDataPropertyOrThrow(_O_, "type", _part_.[[type]]). | ||
1. CreateDataPropertyOrThrow(_O_, "value", _part_.[[value]]). | ||
1. Perform ? DefinePropertyOrThrow(_O_, "value", _valueDesc_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preceding three lines look funny in an identical way to NumberFormat.formatToParts. Do you think you could put the two bodies in a single abstract algorithm (leaving out the first step) to reduce duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that will be true for all .formatToParts()
in the future, including relative time, etc.
77cdef1
to
3c253c1
Compare
LGTM on the new version |
1. Let _fv_ be an implementation and locale dependent String value representing "post meridiem"; | ||
1. Else, | ||
1. Let _fv_ be an implementation and locale dependent String value representing "ante meridiem". | ||
1. Add new part record { [[type]]: *"dayPeriod"*, [[value]]: _fv_ } as a new element of the list _result_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dayPeriods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why dayPeriods
? We call the token dayPeriod
- https://github.com/andyearnshaw/Intl.js/blob/master/src/12.datetimeformat.js#L1011
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I got confused, look two lines above the link you provided, it is using dayPeriods, so I was confused here. we need to update the polyfill then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
The internal resolveDateString
call uses dayPeriods
, both before formatToParts landed [0] and after.
The public token name is dayPeriod
.
What do you need me to update in the polyfill?
[0] andyearnshaw/Intl.js@a9743c3#diff-7eb52b366866677666470e019283c8eaL2489
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is confusing to have two things that are so similar, dayPeriod
and dayPeriods
. But I will review it myself, see if that makes sense or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, in CLDR we have a list dayPeriods
- https://github.com/andyearnshaw/Intl.js/blob/2fd2654d2adc8f870fa3f0616707d333135cc9f1/locale-data/json/en-US.json#L168
And then in formatToParts we create a token with a single dayPeriod
.
76adfa8
to
09c03c2
Compare
Heads up, I rebased on top of master and moved |
09c03c2
to
bcf6ca0
Compare
LGTM @zbraniecki, now we should wait for consensus at TC39 before merging this. |
Not just consensus at TC39 but Stage 4, with two implementations (I'd prefer two browser implementations, personally). |
bcf6ca0
to
9a0e35b
Compare
@littledan - we now have two implementations - one in SpiderMonkey and one in Intl.js. I'd love to see two browser implementations but that depends on the implementers. Do you know when we can see the feature in V8? |
9a0e35b
to
934677d
Compare
With test262 in pipeline, I'm planning to ask for the second implementer to step up at the current TC39 meeting. I'd like to break the chicken-egg problem between 2 impl. and stage 4. |
1. Let _O_ be ObjectCreate(%ObjectPrototype%). | ||
1. Perform ? CreateDataPropertyOrThrow(_O_, "type", _part_.[[type]]). | ||
1. Perform ? CreateDataPropertyOrThrow(_O_, "value", _part_.[[value]]). | ||
1. Perform ? CreateDataProperty(_result_, ? ToString(_n_), _O_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToString(_n_)
will not throw. ?
can be replaced by !
.
Actually I believe none of these last 3 lines will throw anything. All of the ?
can be replaced by !
on them.
merged as 4a711f0 |
Issue: #30
Features
CreateDateTimeParts
is a new abstract that powersformat()
andformatToParts()
fordataTimeFormat
instances.