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

Stage 4 PR for proposal-intl-extend-timezonename #621

Merged
merged 2 commits into from
Dec 30, 2021

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Nov 4, 2021

Based on https://tc39.es/proposal-intl-extend-timezonename/

The <del> and some <ins> in the spec text is already in latest ECMA402

pending on Stage 4 Advancement in 2021-Dec TC39

Based on https://tc39.es/proposal-intl-extend-timezonename/

The <del> and some <ins> in the spec text is already in latest ECMA402
Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM, a few nits inline.

BTW, the diff at https://tc39.es/proposal-intl-extend-timezonename/ is outdated around FormatDateTimePattern pt 15.e.

1. Else if _property_ is *"timeZoneName"*, then
1. If _optionsProp_ is *"short"* or *"shortGeneric"*, then
1. If _formatProp_ is *"shortOffset"*, decrease _score_ by _offsetPenalty_.
1. Else if _formatProp_ is *"longOffset"*, decrease _score_ by (_offsetPenalty_ + _shortMorePenalty_).
Copy link
Member

Choose a reason for hiding this comment

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

nit: are the brackets necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that make it clearer

spec/datetimeformat.html Outdated Show resolved Hide resolved
@FrankYFTang
Copy link
Contributor Author

LGTM, a few nits inline.

BTW, the diff at https://tc39.es/proposal-intl-extend-timezonename/ is outdated around FormatDateTimePattern pt 15.e.

I know, I landed dbe5d72#diff-aae61752e383a82db0847140fc85c3f1718f6348407153200b6db8462f10a21a myself. So the diff in this PR is only the diff from the spec text.

@ryzokuken
Copy link
Member

@FrankYFTang great, thanks. Consider my questions resolved.

@gibson042 @leobalter do you folks want to take a quick look?

Good luck for Stage 4!

spec/datetimeformat.html Outdated Show resolved Hide resolved
Comment on lines 286 to 301
1. Else if _property_ is *"timeZoneName"*, then
1. If _optionsProp_ is *"short"* or *"shortGeneric"*, then
1. If _formatProp_ is *"shortOffset"*, decrease _score_ by _offsetPenalty_.
1. Else if _formatProp_ is *"longOffset"*, decrease _score_ by (_offsetPenalty_ + _shortMorePenalty_).
1. Else if _optionsProp_ is *"short"* and _formatProp_ is *"long"*, decrease _score_ by _shortMorePenalty_.
1. Else if _optionsProp_ is *"shortGeneric"* and _formatProp_ is *"longGeneric"*, decrease _score_ by _shortMorePenalty_.
1. Else if _optionsProp_ ≠ _formatProp_, decrease _score_ by _removalPenalty_.
1. Else if _optionsProp_ is *"shortOffset"* and _formatProp_ is *"longOffset"*, decrease _score_ by _shortMorePenalty_.
1. Else if _optionsProp_ is *"long"* or *"longGeneric"*, then
1. If _formatProp_ is *"longOffset"*, decrease _score_ by _offsetPenalty_.
1. Else if _formatProp_ is *"shortOffset"*, decrease _score_ by (_offsetPenalty_ + _longLessPenalty_).
1. Else if _optionsProp_ is *"long"* and _formatProp_ is *"short"*, decrease _score_ by _longLessPenalty_.
1. Else if _optionsProp_ is *"longGeneric"* and _formatProp_ is *"shortGeneric"*, decrease _score_ by _longLessPenalty_.
1. Else if _optionsProp_ ≠ _formatProp_, decrease _score_ by _removalPenalty_.
1. Else if _optionsProp_ is *"longOffset"* and _formatProp_ is *"shortOffset"*, decrease _score_ by _longLessPenalty_.
1. Else if _optionsProp_ ≠ _formatProp_, decrease _score_ by _removalPenalty_.
Copy link
Contributor

Choose a reason for hiding this comment

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

@FrankYFTang this works, but I would love for the adjustments to be expressed more concisely. What would you think of this?

Suggested change
1. Else if _property_ is *"timeZoneName"*, then
1. If _optionsProp_ is *"short"* or *"shortGeneric"*, then
1. If _formatProp_ is *"shortOffset"*, decrease _score_ by _offsetPenalty_.
1. Else if _formatProp_ is *"longOffset"*, decrease _score_ by (_offsetPenalty_ + _shortMorePenalty_).
1. Else if _optionsProp_ is *"short"* and _formatProp_ is *"long"*, decrease _score_ by _shortMorePenalty_.
1. Else if _optionsProp_ is *"shortGeneric"* and _formatProp_ is *"longGeneric"*, decrease _score_ by _shortMorePenalty_.
1. Else if _optionsProp_ ≠ _formatProp_, decrease _score_ by _removalPenalty_.
1. Else if _optionsProp_ is *"shortOffset"* and _formatProp_ is *"longOffset"*, decrease _score_ by _shortMorePenalty_.
1. Else if _optionsProp_ is *"long"* or *"longGeneric"*, then
1. If _formatProp_ is *"longOffset"*, decrease _score_ by _offsetPenalty_.
1. Else if _formatProp_ is *"shortOffset"*, decrease _score_ by (_offsetPenalty_ + _longLessPenalty_).
1. Else if _optionsProp_ is *"long"* and _formatProp_ is *"short"*, decrease _score_ by _longLessPenalty_.
1. Else if _optionsProp_ is *"longGeneric"* and _formatProp_ is *"shortGeneric"*, decrease _score_ by _longLessPenalty_.
1. Else if _optionsProp_ ≠ _formatProp_, decrease _score_ by _removalPenalty_.
1. Else if _optionsProp_ is *"longOffset"* and _formatProp_ is *"shortOffset"*, decrease _score_ by _longLessPenalty_.
1. Else if _optionsProp_ ≠ _formatProp_, decrease _score_ by _removalPenalty_.
1. Else if _property_ is *"timeZoneName"*, then
1. Let _penalty_ be the value of the column with name matching _formatProp_ in the row of <emu-xref href="#table-timezonename-component-non-default-penalties"></emu-xref> for which the value of the &ldquo;Requested Style&rdquo; column is _optionsProp_. If that cell is empty, let _penalty_ be _removalPenalty_.
1. If _optionsProp_ = _formatProp_, then
1. Assert: _penalty_ is 0.
1. Else,
1. Decrease _score_ by _penalty_.
<emu-table id="table-timezonename-component-non-default-penalties">
  <emu-caption>*"timeZoneName"* component non-default penalties</emu-caption>
Requested Style *"short"* *"long"* *"shortOffset"* *"longOffset"* *"shortGeneric"* *"longGeneric"*
*"short"* 0 _shortMorePenalty_ _offsetPenalty_ _offsetPenalty_ + _shortMorePenalty_
*"shortGeneric"* _offsetPenalty_ _offsetPenalty_ + _shortMorePenalty_ 0 _shortMorePenalty_
*"shortOffset"* 0 _shortMorePenalty_
*"long"* _longLessPenalty_ 0 _offsetPenalty_ + _longLessPenalty_ _offsetPenalty_
*"longGeneric"* _offsetPenalty_ + _longLessPenalty_ _offsetPenalty_ _longLessPenalty_ 0
*"longOffset"* _longLessPenalty_ 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am nervous about this suggestion. This is a much bigger (pure editorial I assume) changes. How about we merge it w/o such into ECMA402 and then later put that into a different Editorial PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't object either way. But for clarity: would you support such a change, or do you prefer the algorithm steps as they are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me put down this way- I am supporting change it to a table form as an editorial change but I think we need more spend more time and review from more people (in particular @anba) for that PR to make sure we didn't change unintentionally. So I prefer not mix that into this Stage 4 PR.

Co-authored-by: Ujjwal Sharma <ryzokuken@disroot.org>
Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM. I like the change proposed by @gibson042 but we can do it as an editorial ECMA-402 PR after merging this, as @FrankYFTang proposed.

@ryzokuken
Copy link
Member

Since we've hit stage 4 and all editorial issues have been addressed, can we merge this?

@FrankYFTang
Copy link
Contributor Author

Since we've hit stage 4 and all editorial issues have been addressed, can we merge this?

@ryzokuken Who are you asking this question to? Are you asking @gibson042 ? I do not have merge right since I am not editor so we need someone have merge right to click the button to merge.

@ryzokuken
Copy link
Member

@FrankYFTang I have merge access, I just wanted to confirm that this PR is good to go before doing that.

@FrankYFTang
Copy link
Contributor Author

I just wanted to confirm that this PR is good to go before doing that.

Are you asking @gibson042 ? or are you asking me? or someone else?

@ryzokuken
Copy link
Member

@FrankYFTang I was asking both of you what you think. But I believe we're good to go, so since there aren't any objections, I'll merge this.

@ryzokuken ryzokuken merged commit 1ba5ee7 into tc39:master Dec 30, 2021
@FrankYFTang
Copy link
Contributor Author

thanks

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.

3 participants