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

chore(ecto_adatper): Update decoding behavior in adapters for Ecto 3.11 #79

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

caleb-acosta
Copy link

This pull request addresses the changes made in Ecto 3.11 regarding decoding behavior in adapters. Previously, decoding in adapters expected a certain type, but with the update to Ecto 3.11, nil values are introduced during the decoding process.

Source: Ecto 3.11 changelog

defp int_decode(int) when is_binary(int), do: {:ok, String.to_integer(int)}
defp int_decode(int), do: {:ok, int}

defp time_decode(decode), do: {:ok, nil}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it meant nil instead of decode here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this perplexed me a bit too, even if its not totally necessary, I think if I'm understanding correctly the explicit match would be good here for clarity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops! fixed 😵‍💫

In Ecto 3.11, adapters now handle decoding with nil values, originating from null values in records.
@PM-Pepsico
Copy link
Contributor

We should bump the minimum version of Ecto to at least 3.11 in the mix.exs file to go along with this change.

@caleb-acosta
Copy link
Author

We should bump the minimum version of Ecto to at least 3.11 in the mix.exs file to go along with this change.

@PM-Pepsico Can updating the minimum Ecto version potentially force projects dependent on this to update their Ecto versions? I don't see these changes requiring an Ecto update, or am I misunderstanding something?

@PM-Pepsico
Copy link
Contributor

You are correct, I was thinking it would create an issue with older versions.

You are definitely right though, this should remain backwards compatible.

I'd say go ahead and merge it in.

@caleb-acosta caleb-acosta merged commit 6504bb4 into v1.0.0 Dec 6, 2023
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.

4 participants