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

Minutes of negative offset also need multiplier #31

Open pd9333 opened 2 years ago

pd9333 commented 2 years ago

Code here: https://github.com/rtfeldman/elm-iso8601-date-strings/blob/3587ff12251261514d0da49837f3c170d0249448/src/Iso8601.elm#L342-L345

Might be:

multiplier * (hours * 60 + minutes)
jamesrweb commented 1 year ago

@pd9333 can you give an example test case to show where utcOffsetMinutesFromParts doesn't work as intended per specification?

pd9333 commented 1 year ago

Based on wikipedia, it looks like there are currently only two areas which are using negative offsets and has minutes

UTC offset Location
UTC−09:30 French Polynesia: Marquesas Islands
UTC−03:30 Canada: Newfoundland, Labrador (southeast)

Take the first one for example. If we try to parse -09:30, the correct value should be -570, but current implementation gives -510

jamesrweb commented 1 year ago

Based on wikipedia, it looks like there are currently only two areas which are using negative offsets and has minutes

| UTC offset | Location |

| -- | -- |

| UTC−09:30 | French Polynesia: Marquesas Islands |

| UTC−03:30 | Canada: Newfoundland, Labrador (southeast) |

Take the first one for example. If we try to parse -09:30, the correct value should be -570, but current implementation gives -510

I will take a look into this later today, thanks for the context!

jamesrweb commented 1 year ago

@pd9333 Opened PR #32 and now it is for @rtfeldman to review and merge. Really nice catch, kudos to you! 🎉

pd9333 commented 1 year ago

:bow: