Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tfiedlerdejanze
Copy link

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 uses DateTime.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:

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
Copy link
Author

@bitwalker is this a change you'd be considering for Timex?

lib/datetime/datetime.ex Outdated Show resolved Hide resolved
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]
```
@@ -447,13 +447,17 @@ defimpl Timex.Protocol, for: DateTime do
err

%DateTime{} = datetime when shift != 0 ->
Copy link

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)
Copy link

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
Copy link

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. 🤷 😅

@ftes
Copy link

ftes commented May 15, 2023

@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!

@petermueller
Copy link

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.
We thankfully have a shim layer where we can perform this for as well, but would love to see this merged to make our testing a little easier

@edennis
Copy link

edennis commented Oct 10, 2023

Any chance of getting this merged? 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timex.shift/2 with time-shift always converts precision to :microsecond in Elixir v1.14+
5 participants