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

Document requirement for UNIX timestamps to be correctly converted #536

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@bjrnt

bjrnt commented Mar 25, 2016

When playing around in the repl, I found that typical UNIX timestamps (e.g. 1458297862) do not give the expected result when using Date.fromTime, even though it seems like it would from the documentation. After asking on Stack Overflow, I learned that the timestamps must include milliseconds for the conversion to be correct.

Example:

new Date(1458297862) 
"Sun Jan 18 1970 06:04:57 GMT+0900 (KST)"
new Date(1458297862000)
"Fri Mar 18 2016 19:44:22 GMT+0900 (KST)"

I think it would be good to clarify this in the documentation, as the behavior is rather counter-intuitive.

@bjrnt bjrnt changed the title from Document requirement for UNIX timestamps to be converted to Document requirement for UNIX timestamps to be correctly converted Mar 25, 2016

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 25, 2016

Contributor

While maybe the documentation does deserve some improvement here, I find your proposal problematic.

Two things:

  1. What does it really mean to "include" milliseconds? Isn't it more an issue of "being measured in" milliseconds?
  2. The claim that the timestamp should have 13 digits is obviously wrong. If, for example, we are talking about a date in 1970, then it won't be 13 digits.

The real problem here seems to be the references to "UNIX time", which is usually measured in seconds. But there is nothing UNIXy here. Elm has the Time type in http://package.elm-lang.org/packages/elm-lang/core/3.0.0/Time, and that type is the type which is used in Date.toTime and Date.fromTime, and the Time type itself does deliberately not say anything like "this is measured in milliseconds". Instead, the Time module has an API with millisecond, second, minute, hour : Time and inMilliseconds, inSeconds, inMinutes, inHours : Time -> Float. Those should be used, and the seeming problem you observe disappears.

That is, don't use Date.fromTime 1458297862, but instead Date.fromTime (1458297862 * second), and don't use plain Date.toTime someDate, but instead inMilliseconds (Date.toTime someDate). That will resolve all possible confusion issues.

Contributor

jvoigtlaender commented Mar 25, 2016

While maybe the documentation does deserve some improvement here, I find your proposal problematic.

Two things:

  1. What does it really mean to "include" milliseconds? Isn't it more an issue of "being measured in" milliseconds?
  2. The claim that the timestamp should have 13 digits is obviously wrong. If, for example, we are talking about a date in 1970, then it won't be 13 digits.

The real problem here seems to be the references to "UNIX time", which is usually measured in seconds. But there is nothing UNIXy here. Elm has the Time type in http://package.elm-lang.org/packages/elm-lang/core/3.0.0/Time, and that type is the type which is used in Date.toTime and Date.fromTime, and the Time type itself does deliberately not say anything like "this is measured in milliseconds". Instead, the Time module has an API with millisecond, second, minute, hour : Time and inMilliseconds, inSeconds, inMinutes, inHours : Time -> Float. Those should be used, and the seeming problem you observe disappears.

That is, don't use Date.fromTime 1458297862, but instead Date.fromTime (1458297862 * second), and don't use plain Date.toTime someDate, but instead inMilliseconds (Date.toTime someDate). That will resolve all possible confusion issues.

@bjrnt

This comment has been minimized.

Show comment
Hide comment
@bjrnt

bjrnt Mar 25, 2016

Thanks for your feedback - I've updated my proposal based on it.

Although there's nothing UNIXy there, I think there's still value in mentioning UNIX time, as people will often want to convert a UNIX timestamp. I also included an example that has the milliseconds tacked on to the end, so hopefully that will clear up any confusion.

bjrnt commented Mar 25, 2016

Thanks for your feedback - I've updated my proposal based on it.

Although there's nothing UNIXy there, I think there's still value in mentioning UNIX time, as people will often want to convert a UNIX timestamp. I also included an example that has the milliseconds tacked on to the end, so hopefully that will clear up any confusion.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 25, 2016

Contributor

I don't see "an example that has the milliseconds tacked on to the end" in the latest version of yours. Also, I don't think "UNIX time measured in milliseconds" is what this function takes. It takes a Time.Time, full stop.

Also, I wonder why people in the browser "will often want to convert a UNIX timestamp". Where would they get that from? Isn't it more likely that they would want to convert a JavaScript timestamp? In any case, in Elm there's the Time.Time type. That's the relevant point of reference here.

Contributor

jvoigtlaender commented Mar 25, 2016

I don't see "an example that has the milliseconds tacked on to the end" in the latest version of yours. Also, I don't think "UNIX time measured in milliseconds" is what this function takes. It takes a Time.Time, full stop.

Also, I wonder why people in the browser "will often want to convert a UNIX timestamp". Where would they get that from? Isn't it more likely that they would want to convert a JavaScript timestamp? In any case, in Elm there's the Time.Time type. That's the relevant point of reference here.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 25, 2016

Contributor

Ah, by "milliseconds tacked on to the end" you mean the last three zeros? If anything, that example should be 646109100 * second. Just take seriously what the documentation says about Time.Time:

Using the Time constants instead of raw numbers is very highly recommended.

Contributor

jvoigtlaender commented Mar 25, 2016

Ah, by "milliseconds tacked on to the end" you mean the last three zeros? If anything, that example should be 646109100 * second. Just take seriously what the documentation says about Time.Time:

Using the Time constants instead of raw numbers is very highly recommended.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 25, 2016

Contributor

I'll merge this if you:

  • drop the "measured in milliseconds"
  • replace 646109100000 by 646109100 * second
  • squash all the commits into one
Contributor

jvoigtlaender commented Mar 25, 2016

I'll merge this if you:

  • drop the "measured in milliseconds"
  • replace 646109100000 by 646109100 * second
  • squash all the commits into one
@bjrnt

This comment has been minimized.

Show comment
Hide comment
@bjrnt

bjrnt Mar 25, 2016

All done.

There are some APIs that return UNIX timestamps. The one I was interacting with was reddits.

Thanks for looking my PR over and providing feedback!

bjrnt commented Mar 25, 2016

All done.

There are some APIs that return UNIX timestamps. The one I was interacting with was reddits.

Thanks for looking my PR over and providing feedback!

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 25, 2016

Contributor

Hmm, well, you didn't:

  • drop the "measured in milliseconds"
Contributor

jvoigtlaender commented Mar 25, 2016

Hmm, well, you didn't:

  • drop the "measured in milliseconds"

@bjrnt bjrnt closed this Mar 25, 2016

@bjrnt bjrnt deleted the bjrnt:patch-2 branch Mar 25, 2016

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 25, 2016

Contributor

Thanks for bringing this issue up, and sorry you just left.

I try to address this in https://github.com/elm-lang/core/pull/538 now.

Contributor

jvoigtlaender commented Mar 25, 2016

Thanks for bringing this issue up, and sorry you just left.

I try to address this in https://github.com/elm-lang/core/pull/538 now.

@bjrnt

This comment has been minimized.

Show comment
Hide comment
@bjrnt

bjrnt Mar 25, 2016

Thanks for saying that. Just had a horrible day.
I'll be back with another, better PR soon. 😉

bjrnt commented Mar 25, 2016

Thanks for saying that. Just had a horrible day.
I'll be back with another, better PR soon. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment