fuelen/ecto_dev_logger

Comments for numeric enum

Closed this issue · 4 comments

First of all - thank you for the great project.
I Just faced an issue that also gave me an idea - the generated sql looks like this:
sp0."entry_type" = ANY ('{10,11,13}')
I think this is from such ecto code:
where([sp], sp.entry_type in [:green, :blue, :red]) where the values ar Ecto.Enums
First of all it fails for me when executing.
Second - could we add a comment after it that points to the actual enum value?
sp0."entry_type" = ANY (10,11,13) /* green, blue, red */

Hi Daniel

First of all it fails for me when executing.

Why does it fail? What error do you see?

Second - could we add a comment after it that points to the actual enum value?

This is an interesting idea. However, It can't be implemented with the current interface of Ecto.DevLogger.PrintableParameter. We have to implement something smarter :)

Sorry, I think I messed something up because when I retried it now it works.
I was thinking it's invalid sql but I just double checked it and it's works.

Hey @fuelen, I just looked how much effort it would take and I think it may not be so hard:
In the telemetry handler we can zip the params with cast_params and use it in the inline_params function.
params = Enum.zip(metadata.params, metadata.cast_params || []) <- cast_params is nil when there are no params - this should be probably fixed in ecto_sql.

This way the function receives a tuple {[1,5],[:foo, :baz]} when running this test.

test "enum field" do                                                                                                                
   enum_value = [:foo, :baz]                                                                                                         
   post = %Post{enum: enum_value} |> Repo.insert!()                                                                                  
   assert Repo.get(Post, post.id).enum == enum_value                                                                                 
end       

Hi @dkuku

Yes, this is an option.
However, I think, we'd have to extend Ecto.DevLogger.PrintableParameter to do this. And this requires a major version bump