Fix: preserve datetime precision after Timex.shift/2 #741
Conversation
tfiedlerdejanze
force-pushed
the
patch-1
branch
from
dc2b930 to
98394f7
Compare
|
@bitwalker is this a change you'd be considering for Timex? |
Resolves bitwalker#731 Unfortunately this broke with elixir-lang/elixir@5a583c7 which was release with Elixir 1.14.3 Elixir 1.13.4: ``` iex(3)> DateTime.utc_now() |> IO.inspect() |> DateTime.truncate(:second) |> IO.inspect() |> Timex.shift(minutes: 1) |> IO.inspect() ~U[2023-04-13 09:56:10.136274Z] ~U[2023-04-13 09:56:10Z] ~U[2023-04-13 09:57:10Z] ``` Elixir 1.14.4: ``` iex(1)> DateTime.utc_now() |> IO.inspect() |> DateTime.truncate(:second) |> IO.inspect() |> Timex.shift(minutes: 1) |> IO.inspect() ~U[2023-04-13 09:55:16.405357Z] ~U[2023-04-13 09:55:16Z] ~U[2023-04-13 09:56:16.000000Z] ```
tfiedlerdejanze
force-pushed
the
patch-1
branch
from
98394f7 to
4c00c0b
Compare
| @@ -447,13 +447,17 @@ defimpl Timex.Protocol, for: DateTime do | |||
| err | |||
|
|
|||
| %DateTime{} = datetime when shift != 0 -> | |||
There was a problem hiding this comment.
Hope you don't mind if I make further suggestions. How about rewriting the two arrow clauses, like so:
%DateTime{} = original ->
if shift == 0 do
original
else
original
|> DateTime.add(shift, :microsecond, Timex.Timezone.Database)
|> retain_precision(original)
end| DateTime.add(orig, shift, :microsecond, Timex.Timezone.Database) | ||
| {{ty, _, _}, %DateTime{} = original} when ty in [:gap, :ambiguous] and shift != 0 -> | ||
| original | ||
| |> DateTime.add(shift, :microsecond, Timex.Timezone.Database) |
There was a problem hiding this comment.
Maybe it's a good idea to create a function for adding microseconds while keeping the precision? Something like:
original |> add_with_precision_preserved(shift)| {{ty, _, _}, %DateTime{} = original} when ty in [:gap, :ambiguous] and shift != 0 -> | ||
| original | ||
| |> DateTime.add(shift, :microsecond, Timex.Timezone.Database) | ||
| |> retain_precision(datetime) | ||
|
|
||
| {{ty, _a, _b} = amb, _} when ty in [:gap, :ambiguous] -> | ||
| amb |
There was a problem hiding this comment.
I would get rid of the case statement below as it seems completely unnecessary, but that may be outside of the scope of this PR. 🤷 😅
|
@bitwalker Do you have any time plans for reviewing/merging this PR? "I don't have time" is also a totally fine answer that helps us plan accordingly. Thank you for your efforts! |
|
Thank you doing this work. I've been unable to complete an upgrade to 1.14 as a huge number of our legacy database tables were created w/ resolution only down to the second. |
|
Any chance of getting this merged? 🙏🏻 |
Summary of changes
Resolves #731
Unfortunately this broke with elixir-lang/elixir@5a583c7 which was released with Elixir 1.14.3
As
Timex.shift/2calculates the total microseconds of the shift options and then usesDateTime.add/4to apply, Elixir returns a new datetime with the highest precision it finds (microseconds) opposed to keeping the original precision.Elixir 1.13.4:
Elixir 1.14.4: