`to_datetime` doesn't accept 4-tuple time

[martinsvalin]

This is actually two issues, but there's only so much room in a title.

Timex.Types.time can be either 3-tuple or 4-tuple. The spec for Timex.to_datetime says it accepts datetime tuples, but it only implements for 3-tuples.

I would PR a fix, but when I started to look into it, I realised a more nefarious issue. The 4-tuple time type has milliseconds in the last position, not microseconds. The spec for Timex.DateTime.Helpers.construct says it accepts datetime tuples, same as to_datetime, but the implementation thinks it's getting microseconds in the last position of the 4-tuple.

So there's some confusion whether a 4-tuple time has milli- or microsecond units. Either the Type needs to be updated to microseconds or construct needs to handle milliseconds. I don't know which. When all the types match up, the fix for my original issue will be trivial.

Here's a test case for the to_datetime issue, assuming 4-tuples have milliseconds:

datetime = Timex.to_datetime({{2015,2,28}, {12,31,2,900}}, "Etc/UTC")
assert ^datetime = Timex.to_datetime(~N[2015-02-28T12:31:02.900000], "Etc/UTC")

I'll gladly help out with implementation, but need some guidance with the units.

[bitwalker]

The type for the 4-tuple date/time should be removed. I'm not planning to support 4-tuples since the ambiguity between milliseconds/microseconds means that it's not particularly useful. With 1.3, it's better to construct Date/NaiveDateTime/DateTime using the new Calendar API, or 3-tuples with the :calendar API, rather than constructing tuples manually and passing them to Timex. As far as compatibility with Erlang APIs, :calendar doesn't create any 4-tuples, so there's not really a good reason to support them to begin with.

The old Timex API took 4-tuples with milliseconds, but that was because at that time, there wasn't really support for microseconds generally, and Timex was more or less the date/time library, so some assumptions could be made. Now that 1.3 has calendar types, and uses microseconds for sub-second precision, it is more confusing to support milliseconds than microseconds, and additionally, due to the fact that Timex previously took 4-tuples with milliseconds, changing it to take 4-tuples with microseconds would likely result in errors as previous users of Timex would be expecting it to take milliseconds. While the typespecs would indicate what is expected, in my experience, people rarely check those. I would rather the API remain unambiguous. If it's important that you be able to construct date/times with 4-tuples, Timex.DateTime.Helpers.construct can be used, though it's not intended to be a public API. It is very unlikely to change though, since it's a low-level function used throughout Timex. In general though I would recommend using NaiveDateTime.new in conjunction with Timex.to_datetime/2 if you need to construct DateTimes which are high precision. In terms of efficiency, the difference is effectively nil anyway.

I'll leave this open for feedback, but for the time being I'm going to consider this "wontfix", other than the typespec change which I'll do now.

[martinsvalin]

Thanks for the reply. Dropping 4-tuples from the time spec sounds completely reasonable, and in that case there's no point adding support for them in to_datetime. I looked into this after a chat on the Elixir Slack, https://elixir-lang.slack.com/archives/general/p1470056747008242:

thorsten-michael: Are 4-Tuples {hour, minute, second, millisecond} still valid to represent a time? I get them from an Ecto.Query using Mariaex, but Timex.to_datetime doesn't like them at all...

So I might continue digging through the Elixir ecosystem for datetimes to see why a 4-tuple is returned. It would be better for everyone if our ecosystem moved to microseconds for interop.

[bitwalker]

I believe Ecto currently represents time with a 4-tuple, perhaps that would be worth looking into. I know Ecto will be moving to calendar types from 1.3 soon though, so I wonder if it's worth adding support for those. Still, it might answer the question of which of the two units to support.

[jcelliott]

FWIW, the Logger module uses a 4-tuple for log timestamps. I just realized this while writing a custom formatter because Timex fails to parse it. See the "Custom Backends" section here: https://hexdocs.pm/logger/Logger.html