-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: preserve datetime precision after Timex.shift/2 #741
base: main
Are you sure you want to change the base?
Conversation
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] ```
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/2
calculates the total microseconds of the shift options and then usesDateTime.add/4
to 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: