search

rtfeldman / elm-iso8601-date-strings

Convert between ISO-8601 date strings and Time.Posix
https://package.elm-lang.org/packages/rtfeldman/elm-iso8601-date-strings/latest
BSD 3-Clause "New" or "Revised" License
34 stars 21 forks source link

Parsing succeeds with a wrong result #3

Closed ohanhi closed 6 years ago

ohanhi commented 6 years ago

The gist of it is the issue is this:

dateString =
    "2018-07-29T15:00:00.000Z"

parsedAndEncoded =
    dateString
        |> toTime
        |> Result.map fromTime
        -- Ok "2018-07-05T15:00:00.00Z"

I have confirmed that the parsing phase is the source of the issue: the Posix value is incorrect when you turn it into a date with JavaScript's new Date(value). Here's a full example on Ellie.

Let me know if I can help with this.

rtfeldman commented 6 years ago

@ohanhi ha, this is exactly why I was gonna wait until elm-test was upgraded to publish...fuzzing would have caught this right away! That's what I get for being impatient.

At any rate, yeah - if you'd like to PR a fix, I would love that!

justinmimbs commented 6 years ago

I think this issue has to do with the leap year calculation. It should be fixed by this PR: https://github.com/rtfeldman/elm-iso8601/pull/2 :)

rtfeldman commented 6 years ago

@ohanhi can you try version 1.0.1 with @justinmimbs's fix, and close this issue if that fixes it for you?

justinmimbs commented 6 years ago

I just noticed that fromTime only pads millis to 2 digits instead of 3! So round-tripping any time with millis < 100 won't work till that's changed.

rtfeldman commented 6 years ago

Good point! Want to make a PR for that?

justinmimbs commented 6 years ago

Sure thing! https://github.com/rtfeldman/elm-iso8601/pull/4

rtfeldman commented 6 years ago

Published both fixes as 1.0.2!

ohanhi commented 6 years ago

And I can confirm this now works on my example, so I will close this issue! (Ellie)