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

[4th Edition] DateTimeFormat.formatToParts() #64

Closed
wants to merge 1 commit into from

Conversation

zbraniecki
Copy link
Member

Issue: #30

Features

  • CreateDateTimeParts is a new abstract that powers format() and formatToParts() for dataTimeFormat instances.
  • Moving all abstracts to the new Abstract section.


<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:
Copy link
Contributor

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:

@caridy
Copy link
Contributor

caridy commented Feb 11, 2016

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_.
Copy link
Contributor

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")?

@caridy
Copy link
Contributor

caridy commented Feb 18, 2016

Plus, _dateTimneFormat_ has a typo, in two places.


<emu-clause id="alg-CreateDateTimeParts">

<h1>CreateDateTimeParts(dateTimeFormat, x)</h1>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #79 @caridy wanted to use the verb format for a similar abstract operation: FormatNumberToParts. Perhaps we could rename this here as well for consistency to FormatDateTimeToParts? This will also be more inline with the name of the existing abstract operation: FormatDateTime.

@caridy caridy changed the title DateTimeFormat.formatToParts() [4rd Edition] DateTimeFormat.formatToParts() Feb 29, 2016
@zbraniecki
Copy link
Member Author

@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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the id here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thnx! fixed

@zbraniecki zbraniecki force-pushed the format-to-parts branch 2 times, most recently from 68fa6ae to ca27e01 Compare March 23, 2016 22:15
1. Let _O_ be ObjectCreate(%ObjectPrototype%).
1. CreateDataPropertyOrThrow(_O_, "type", _part_.[[type]]).
1. CreateDataPropertyOrThrow(_O_, "value", _part_.[[value]]).
1. Perform ? DefinePropertyOrThrow(_O_, "value", _valueDesc_).
Copy link
Member

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?

Copy link
Contributor

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.

@zbraniecki zbraniecki force-pushed the format-to-parts branch 2 times, most recently from 77cdef1 to 3c253c1 Compare March 29, 2016 22:37
@littledan
Copy link
Member

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_.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dayPeriods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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 :)

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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.

@zbraniecki zbraniecki force-pushed the format-to-parts branch 2 times, most recently from 76adfa8 to 09c03c2 Compare May 6, 2016 01:53
@zbraniecki
Copy link
Member Author

Heads up, I rebased on top of master and moved prototype.format* functions to throw if the params are wrong instead of asserting.

@caridy
Copy link
Contributor

caridy commented Jun 9, 2016

LGTM @zbraniecki, now we should wait for consensus at TC39 before merging this.

@littledan
Copy link
Member

Not just consensus at TC39 but Stage 4, with two implementations (I'd prefer two browser implementations, personally).

@zbraniecki
Copy link
Member Author

@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?

@zbraniecki
Copy link
Member Author

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_).
Copy link
Member

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.

caridy added a commit that referenced this pull request Aug 14, 2016
caridy added a commit that referenced this pull request Aug 14, 2016
* NumberFormat.prototype.formatToParts()
* DateTimeFormat.prototype.formatToParts()
@caridy
Copy link
Contributor

caridy commented Aug 14, 2016

merged as 4a711f0

@caridy caridy closed this Aug 14, 2016
@rxaviers rxaviers changed the title [4rd Edition] DateTimeFormat.formatToParts() [4th Edition] DateTimeFormat.formatToParts() Aug 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants