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

Add TZOffset{UTC,Local} in place of LocalTZA/DSTA #771

Closed
wants to merge 2 commits into from

Conversation

jungshik
Copy link
Contributor

@jungshik jungshik commented Jan 20, 2017

Currently, LocalTimezoneAdjustment is assumed to be constant across time
zone rule changes and does not take any argument. Daylight Savings Time
Adjustment does take an argument t.

DSTA(t) can 'absorb' the standard time zone offset changes (e.g. Europe/Moscow
changed the standard time zone offset multiple times in the last decade.). However,
the spec stipulates that it should only reprsent an additional
adjustment necessary when DST is in effect.

In practice, at least two implementations (Spidermonkey/Firefox and
JSC/Safari) do not follow the spec to the letter to produce results
expected by users across time zone rule changes.

This PR drops LocalTZA and DaylightSavingsTimeAdjustment(t) and
introduce TZOffsetUTC(t) and TZOffsetLocal(t_local). The former takes
't' (UTC) and gives a time zone offset in effect at t for the local
time zone (for US ET, it'd be 4*msPerHour in summer and 5*msPerHour in
winter). The latter takes 't_local' (local time value) and gives a time
zone offset in effect at t_local.

It's also specified that TZOffsetLocal(t_local) will return the offset
after the transition when t_local is wall time repeating multiple
times at a negative transition (e.g. fall backward) or skipped wall time
at a positive time zone transition (e.g. spring forward). This is in
line with the aforementioned two implementations. In the future, it
may as well be considered to introduce options to control this behavior.
[1] [2]

UTC(t_local) and Localtime(t) are reformulated with
TZOffsetLocal(t_local) and TZOffsetUTC(t).

Will fix #725 modulo an option to specify the behavior to handle
skipped or repeated wall time.

[1] ICU Calendar API has skipped wall time option (https://goo.gl/h0bP26) and
repeated wall time option (https://goo.gl/Q1VX3j).

[2] Python proposal to handle skipped/repeated time:
https://www.python.org/dev/peps/pep-0495/

Currently, LocalTimezoneAdjustment is assumed to be constant across time
zone rule changes and does not take any argument. Daylight Savings Time
Adjustment does take an argument t.

DSTA(t) can 'absorb' the standard time zone offset changes (e.g. Europe/Moscow
changed the standard time zone offset multiple times in the last decade.). However,
the spec stipulates that it should only reprsent an additional
adjustment necessary when DST is in effect.

In practice, at least two implementations (Spidermonkey/Firefox and
JSC/Safari) do not follow the spec to the letter to produce results
expected by users across time zone rule changes.

This PR drops LocalTZA and DaylightSavingsTimeAdjustment(t) and
introduce TZOffsetUTC(t) and TZOffsetLocal(t_local). The former takes
't' (UTC) and gives a time zone offset in effect at t for the local
time zone (for US ET, it'd be 4*msPerHour in summer and 5*msPerHour in
winter). The latter takes 't_local' (local time value) and gives a time
zone offset in effect at t_local.

It's also specified that TZOffsetLocal(t_local) will return the offset
*after* the transition when t_local is wall time repeating multiple
times at a negative transition (e.g. fall backward) or skipped wall time
at a positive time zone transition (e.g. spring forward). This is in
line with the aforementioned two implementations. In the future, it
may as well be considered to introduce options to control this behavior.
[1] [2]

UTC(t_local) and Localtime(t) are reformulated with
TZOffsetLocal(t_local) and TZOffsetUTC(t).

Will fix tc39#725 modulo an option to specify the behavior to handle
skipped or repeated wall time.

[1] ICU Calendar API has skipped wall time option (https://goo.gl/h0bP26) and
repeated wall time option (https://goo.gl/Q1VX3j).

[2] Python proposal to handle skipped/repeated time:
    https://www.python.org/dev/peps/pep-0495/
@jungshik
Copy link
Contributor Author

@bterlson, @ediosyncratic, @littledan

Can you take a look? I ended up making a bit more extensive change than I thought at first.

@bterlson, I hope it's not too late for review at a ecma262 meeting next week.

BTW, I may not have done the right thing when making this PR in terms of what file to touch etc. Somehow, 'npm install', 'npm run build' etc didn't work on my Ubuntu (14.x). So, I just edited spec.html trying to follow the mark-up style as faithfully as I can.

Make the sign of Timezone offset consistent throughout the spec
in line with getTimezoneOffset().
@jungshik
Copy link
Contributor Author

The update is to make the sign of TZOffSetUTC and TZOffsetLocal to be the same as that of Date.prototype.getTimezoneOffset. It's unfortunate that the sign of getTimezoneOffset is the opposite of what most people consider 'intuitive'.

An alternative would be not to use 'Offset' in the names of two operations I'm introducing and make the sign be in line with a more common usage (negative for timezone West of the primary meridian and positive for timezones East of the primary meridian) . Instead of 'Offset', 'Adjustment' can be used.

Yet another alternative would be to note that the sign of offsets returned by two operations is the opposite of getTimezoneOffset() and keep the name ('offset') in my PR.

@jungshik
Copy link
Contributor Author

I have another thought. Instead of introducing two new abstract operations in place of LocalTZA and DSTA(t), I can make LocalTZA take two arguments, t and is_local_time. When is_local_time is true, 't' would be interpreted as a 'local time value' in the current time zone, while 't' would be time value in UTC when it's false.

The reason for is_local_time (boolean) is that 't' needs to be interpreted differently depending on whether 't' is UTC or local time in the current time zone.

That way, I don't have to worry about the sign of TZOffSet{UTC,Local} and can also avoid rather clumsy name with UTC or Local at the end.

I'll make a new PR with this update (to avoid clutter with multiple commits). I'll try to get in this afternoon (UTC-0800). Hope it's not too late for TC39 review this week.

@jungshik
Copy link
Contributor Author

I'm closing this PR in favor of #778.

Copy link

@ediosyncratic ediosyncratic left a comment

Choose a reason for hiding this comment

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

I think this is the better starting-point; it won't create backwards-compatibility problems and does let us do the job properly.

<p>An implementation dependent algorithm using best available information on time zones to determine the local daylight saving time adjustment DaylightSavingTA(_t_), measured in milliseconds. An implementation of ECMAScript is expected to make its best effort to determine the local daylight saving time adjustment.</p>
<emu-clause id="sec-time-zone-offset-from-localtime">
<h1>Time Zone Offset from LocalTime</h1>
<p>An implementation of ECMAScript is expected to determine the local time zone offset from local time TZOffsetLocal(<emu-eqn>_t_<sub>local</sub></emu-eqn>) using the best available information on time zones. The time zone offset at the local time <emu-eqn>_t_<sub>local</sub></emu-eqn> is a value <dfn>TZOffsetLocal(<emu-eqn>_t_<sub>local</sub></emu-eqn>)</dfn> measured in milliseconds which when added to the local time represents UTC.</p>

Choose a reason for hiding this comment

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

This function needs a second, optional parameter, by which to sanitize spring-forward gaps and disambiguate fall-back duplications. Given that you want the default behaviour to be "use the offset that was in effect before the transition", that optional parameter needs to select between before and after; so I suggest it be called prior and default to true; it says whether to use the offset in effect before a transition, when the specified time falls into one; if given and false, it selects the offset in effect after the transition.

<emu-clause id="sec-time-zone-offset-from-localtime">
<h1>Time Zone Offset from LocalTime</h1>
<p>An implementation of ECMAScript is expected to determine the local time zone offset from local time TZOffsetLocal(<emu-eqn>_t_<sub>local</sub></emu-eqn>) using the best available information on time zones. The time zone offset at the local time <emu-eqn>_t_<sub>local</sub></emu-eqn> is a value <dfn>TZOffsetLocal(<emu-eqn>_t_<sub>local</sub></emu-eqn>)</dfn> measured in milliseconds which when added to the local time represents UTC.</p>
<p>When <emu-eqn>_t_<sub>local</sub></emu-eqn> represents wall time repeating multiple times at a positive time zone offset transition (e.g. when the daylight savings time ends or the time zone offset is increased) or skipped wall time at a negative time zone offset transitions (e.g. when the daylight savings time starts or the time zone offset is decreased), <emu-eqn>_t_<sub>local</sub></emu-eqn> should be interpreted with the time zone offset after the transition.</p>

Choose a reason for hiding this comment

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

No need for "multiple times" - given that "repeating" implicitly says that - and explicitly saying "multiple times" rather than "twice" suggests more than twice, which does never happen and we have no sane way to cope with it if it did.

also: s/the daylight savings time/daylight-saving time/g
... and, at the end, s/after/before/, but description of the argument "prior" should cover this.

<emu-alg>
1. Return _t_ - LocalTZA - DaylightSavingTA(_t_ - LocalTZA).
1. Return _t_ + TZOffsetLocal(<emu-eqn>_t_<sub>local</sub></emu-eqn>)

Choose a reason for hiding this comment

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

Missing subscript local on first t.

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.

Ambiguities in UTC()'s spec and mis-specification of DaylightSavingTA()
2 participants