Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Throw RangeError in case old style is mixed with new style. #41

Closed
wants to merge 2 commits into from
Closed

Throw RangeError in case old style is mixed with new style. #41

wants to merge 2 commits into from

Conversation

zbraniecki
Copy link
Member

Fixes #40.

@zbraniecki
Copy link
Member Author

@FrankYFTang - wdyt?

@zbraniecki
Copy link
Member Author

@FrankYFTang - I updated the patch to respect hourCycle. Quite frankly, I'm not sure anymore how the spec made sense before, because there's no place where hourCycle gets assigned to hc otherwise.

Anyhoo, now it should work. Can you review? This seems like a last remaining item before Stage 4!

ljharb
ljharb reviewed Jan 27, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@zbraniecki @ljharb
Update spec.html
b3b4ff0 
Co-Authored-By: Jordan Harband <[email protected]>
@anba
Copy link
Contributor

anba commented Jan 29, 2020

I updated the patch to respect hourCycle. Quite frankly, I'm not sure anymore how the spec made sense before, because there's no place where hourCycle gets assigned to hc otherwise.

The change for "Set hc to hourCycle." looks wrong to me, because ResolveLocale gets the "hourCycle" option through opt record and will then copy it into the r record.

@anba
Copy link
Contributor

anba commented Jan 29, 2020

Furthermore the current patch will likely result in always throwing a RangeError, because ToDateTimeOptions fills in default values from https://tc39.es/ecma402/#table-datetimeformat-components. In other words, ToDateTimeOptions also needs to be updated.

@sffc
Copy link

sffc commented Feb 26, 2020

@Ms2ger can you review this?

Ms2ger
Ms2ger reviewed Feb 26, 2020
View reviewed changes
spec.html
@@ -144,6 +146,8 @@ <h1>InitializeDateTimeFormat ( _dateTimeFormat_, _locales_, _options_ )</h1>
1. <ins>Set _dateTimeFormat_.[[TimeStyle]] to _timeStyle_.</ins>
1. <ins>Set _dateTimeFormat_.[[HourCycle]] to _hc_.</ins>
1. <ins>If _dateStyle_ or _timeStyle_ are not *undefined*, then</ins>
1. <ins>If any of the Properties from <emu-xref href="#table-datetimeformat-components"></emu-xref>, not including `hourCycle`, is defined in _options_,</ins>
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of value is options? Is it a JS object? Does this check imply Get() calls?

anba added a commit to anba/proposal-intl-datetime-style that referenced this pull request Mar 31, 2020
@anba
Throw a RangeError when date/timeStyle is mixed with single date/time…
a4f6ccd 
… options

Drive-by change:
- Use standard EcmaSpeak when testing if both date- and timeStyle are undefined.

Fixes tc39#40.

Supersedes tc39#41.
@anba anba mentioned this pull request Mar 31, 2020
Merged
@anba
Copy link
Contributor

anba commented Mar 31, 2020

I've opened #43, which supersedes this pull request.

@FrankYFTang
Copy link
Contributor

Furthermore the current patch will likely result in always throwing a RangeError, because ToDateTimeOptions fills in default values from https://tc39.es/ecma402/#table-datetimeformat-components. In other words, ToDateTimeOptions also needs to be updated.

I agree with @anba This spec change itself won't work. ToDateTimeOptions will cause it always throw RangeError

@zbraniecki zbraniecki closed this Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ljharb ljharb ljharb left review comments

@Ms2ger Ms2ger Ms2ger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Idea about combining dateStyle/timeStyle with other options
6 participants
@zbraniecki @anba @sffc @FrankYFTang @ljharb @Ms2ger