Edge case when parsing 24:00 datetime in latest version

[neslinesli93]

Hi everyone, during an upgrade of a project to elixir 1.11, we updated Timex to the latest version (3.7.5), which resulted in a bunch of tests failing. We have to handle datetimes in this format: 2017-05-24 2400, which come from an external service we do not have control on.

This is a weird format, as the ISO 8601 article on Wikipedia states:

As of ISO 8601-1:2019 midnight may only be referred to as "00:00", corresponding to the beginning of a calendar day. Earlier versions of the standard allowed "24:00" corresponding to the end of a day, but this is explicitly disallowed by the 2019 revision.

However, Timex worked perfectly before the update to 3.7.5... so I made a bunch of tests, using Timex versions ranging from 3.6.4 to 3.7.5, and elixir 1.10/1.11. I've tried calling this function:

Timex.parse("2017-05-20 2400", "{YYYY}-{0M}-{0D} {h24}{0m}") |> elem(1) |> Map.get(:__struct__)

which resulted in the following:

Elixir 1.10:

• 3.6.4 -> returns NaiveDateTime
• 3.7.0 -> raises because it returns :invalid_time or bad_match
• 3.7.1 -> raises because it returns :invalid_time or bad_match
• 3.7.2 -> returns NaiveDateTime
• 3.7.3 -> returns NaiveDateTime
• 3.7.5 -> raises because it returns :invalid_time or bad_match

Elixir 1.11:

• 3.6.4 -> returns NaiveDateTime
• 3.7.0 -> raises because it returns :invalid_time or bad_match
• 3.7.1 -> raises because it returns :invalid_time or bad_match
• 3.7.2 -> returns NaiveDateTime
• 3.7.3 -> returns NaiveDateTime
• 3.7.5 -> raises because it returns :invalid_time or bad_match

At first I thought it was a breaking change between 3.6 and 3.7, but after more digging it looks like a regression to me... I tried digging into the code but it is way too overwhelming at the moment, hopefully I'll be able to do more after some feedback. Thank you :)

EDIT: found the offending line: Time.new returns :invalid_time:

iex(1)> Time.new(23, 0, 0, 0)
{:ok, ~T[23:00:00.000000]}
iex(2)> Time.new(24, 0, 0, 0)
{:error, :invalid_time}

which looks fine to me, and certainly more of an issue with Time itself than timex... what do you think?

[bitwalker]

@neslinesli93 Yes, unfortunately Timex has been careless about validating times in previous versions, so support has been accidental and spotty as you've noted. We adhere to the rules imposed on us by the Time API though, so if we want to support this in the date/time parser, then we would need to enforce that 24:00 is the same as 00:00, and I'm not sure if that's true. Additionally, since this is explicitly rejected by the ISO-8601 standard, I'm not sure we should support it in the ISO-8601 parser (but we could support it generally via explicit format strings).

So in short, we'd need some consensus on what 24 means as an hour before I'm willing to commit to supporting it. If it's the same as 00, then that's pretty straightforward; if it has more complicated semantics, then I don't think it's like to be something we support directly in Timex, and would instead need to be handled by implementing a custom parser. that you can pass to Timex.parse/3