search

mathieuprog / polymorphic_embed

Polymorphic embeds in Ecto
Apache License 2.0
341 stars 63 forks source link

Use Ecto.embedded_dump/2 #21

Closed maennchen closed 3 years ago

maennchen commented 3 years ago

Simplifies Code by using functionality maintained by Ecto

maennchen commented 3 years ago

PS: If that PR is accepted, I'll send another one for Ecto.embeded_load/3

mathieuprog commented 3 years ago

Looks amazing. I'll work on the library in the coming days. Busy on something else now for a little while. Get back to you soon.

maennchen commented 3 years ago

@mathieuprog Any updates?

mathieuprog commented 3 years ago

Sorry for the delay. This is an amazing PR. Can I just know what this is for? def embed_as(_other, _params), do: :self Commenting it out will make the tests still pass. If we can't think of any failing test, we shouldn't have this function clause I think.

maennchen commented 3 years ago

@mathieuprog I seem to have copied that from somewhere. I've removed the second clause.

mathieuprog commented 3 years ago

I would leave the pattern matching to json like you did before. More explicit.

maennchen commented 3 years ago

@mathieuprog We'll have to take any atom as a parameter. I can leave two clauses that both return :dump (one for json, one for _) or one clause like it is now.

maennchen commented 3 years ago

(Default is :self)

mathieuprog commented 3 years ago

What other atom than :json is possible to receive for this library? It will always be :json as to my understanding. That's why all the tests pass using only :json. Try to remove the other clause and it works.

maennchen commented 3 years ago

@mathieuprog Not really. The user can call Ecto.embedded_dump on any schema he has which includes a field using this library.

For example one could write an XML export of a schema using this functionality.

Ecto internally uses it to serialize embed JSON fields with format set to json.

mathieuprog commented 3 years ago

Thanks. So the last commit is correct: we should always let our custom dump be called. Not use :self otherwise nested polymorphic embeds won't work. Did I get this right?

maennchen commented 3 years ago

I think that's correct :+1:

mathieuprog commented 3 years ago

embed_as is still a little obscure to me, and that's why I missed this huge improvement in the code...

You mentioned somewhere you will fix something else? load?

maennchen commented 3 years ago

@mathieuprog I'll probably do a PR for load as well. Might take some time though until I find some free time.