-
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
Stage 4 PR for proposal-intl-extend-timezonename #621
Conversation
Based on https://tc39.es/proposal-intl-extend-timezonename/ The <del> and some <ins> in the spec text is already in latest ECMA402
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.
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_). |
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.
nit: are the brackets necessary 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.
I think that make it clearer
I know, I landed dbe5d72#diff-aae61752e383a82db0847140fc85c3f1718f6348407153200b6db8462f10a21a myself. So the diff in this PR is only the diff from the spec text. |
@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
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_. |
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.
@FrankYFTang this works, but I would love for the adjustments to be expressed more concisely. What would you think of this?
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 “Requested Style” 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 |
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.
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?
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.
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?
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.
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>
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.
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.
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. |
@FrankYFTang I have merge access, 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? |
@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. |
thanks |
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