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

Time zone and DateTime.prototype.toLocaleString #572

Closed
sffc opened this issue May 15, 2020 · 9 comments · Fixed by #586
Closed

Time zone and DateTime.prototype.toLocaleString #572

sffc opened this issue May 15, 2020 · 9 comments · Fixed by #586
Assignees

Comments

@sffc
Copy link
Collaborator

sffc commented May 15, 2020

I think the current semantics are that we use Intl's time zone (the environment time zone) when formatting a DateTime.

However, this could produce unexpected results:

// I start in America/Chicago
console.log(Temporal.now.timeZone());  // "America/Chicago"

// I create a local time in America/New_York
const dateTime = Temporal.now.absolute().inTimeZone("America/New_York");

// Now I format it.  Oops!  The wall clock time is in America/New_York,
// but the time zone rendered in the string is America/Chicago.
console.log(dateTime.toLocaleString("en", { timeZoneName: "long" }));

Possible resolutions:

  1. Ignore. This is a programmer error.
  2. Forbid a time zone from being displayed in DateTime.prototype.toLocaleString.
  3. Add a time zone slot to the data model (Consider alternatives to Temporal.DateTime (ZonedAbsolute) #569).
@sffc sffc added the calendar Part of the effort for Temporal Calendar API label May 15, 2020
@ptomato

This comment has been minimized.

@sffc

This comment has been minimized.

@ptomato
Copy link
Collaborator

ptomato commented May 15, 2020

I see the point about opting in. I am strongly in favour of option 2 (doesn't preclude option 3 if we decide so on that issue).

Mainly because it's in line with what other types do, in the case where you request information that they don't have:

> Temporal.now.time().toLocaleString('en', { month: 'numeric' })
'3:54:52 PM'

@sffc sffc removed the calendar Part of the effort for Temporal Calendar API label May 15, 2020
@justingrant
Copy link
Collaborator

justingrant commented May 16, 2020

IMHO, the current behavior (1) is not OK to ignore. If DateTime doesn't know about its time zone, then formatting it should not include a timezone. Especially not the wrong timezone!

Seems like (2) is the right behavior for a timezone-less class.

Nice to see another use-case that'd get easier with a ZonedAbsolute. ;-)

Related: #577

ptomato added a commit that referenced this issue May 18, 2020
If the formatting options request a time zone name for any type except
Absolute, or a day for YearMonth, or a year for MonthDay, we want to
ignore that, since the data model for that type doesn't contain those
units.

Closes: #572
@ptomato
Copy link
Collaborator

ptomato commented May 18, 2020

I've gone ahead and made a pull request for option 2 since I haven't heard any opposition.

@ptomato ptomato self-assigned this May 18, 2020
ryzokuken pushed a commit that referenced this issue May 19, 2020
If the formatting options request a time zone name for any type except
Absolute, or a day for YearMonth, or a year for MonthDay, we want to
ignore that, since the data model for that type doesn't contain those
units.

Closes: #572
@sffc
Copy link
Collaborator Author

sffc commented May 19, 2020

Should we render the time zone if it is specified explicitly?

console.log(dateTime.toLocaleString("en", { timeZoneName: "long", timeZone: "Asia/Singapore" }));

@sffc sffc reopened this May 19, 2020
@ptomato
Copy link
Collaborator

ptomato commented May 19, 2020

I'd say no, since it could be ambiguous — this was what @justingrant was referring to in #577.

@ryzokuken ryzokuken added this to the Stage 3 milestone May 20, 2020
@ptomato ptomato modified the milestones: Stage 3, Stable API Jun 4, 2020
@ptomato
Copy link
Collaborator

ptomato commented Jun 22, 2020

@sffc Do you feel strongly about rendering the time zone if it is specified explicitly? I think that it should be treated the same as e.g. console.log(Temporal.now.time().toLocaleString("en", { month: 'long' })) which currently ignores the month option since it doesn't exist in the context of Temporal.Time. Feel free to reopen if you disagree.

@ptomato ptomato closed this as completed Jun 22, 2020
@sffc
Copy link
Collaborator Author

sffc commented Jun 22, 2020

I'm fine with this conclusion. Your reasoning sounds good.

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 a pull request may close this issue.

4 participants