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

Added Repo.embedded_load/3 function #3270

Merged
merged 7 commits into from
Apr 6, 2020

Conversation

narrowtux
Copy link
Contributor

as described in #3269

@josevalim
Copy link
Member

Oh, interesting. Given this function doesn't use the adapter, maybe it doesn't belong on Repo but elsewhere. The question is where? Ecto.Schema? Ecto?

@josevalim
Copy link
Member

Btw, really good job. The question is only where to place the code.

@narrowtux
Copy link
Contributor Author

Yeah that's true. I think it belongs in Schema then.

@josevalim
Copy link
Member

@wojtekmach @michalmuskala any thoughts?

@wojtekmach
Copy link
Member

maybe it doesn't belong on Repo but elsewhere. The question is where? Ecto.Schema? Ecto?

Yeah, neither option seems optimal to me. Given embedded_load/3 is implemented and documented almost identically to Repo.load/2, I think I'd keep it on the Repo regardless of the caveat you mentioned (which I totally agree with). I think putting it on Ecto.Schema would be fine too but then let's mention both functions in their respective docs imho.

@michalmuskala
Copy link
Member

Yes, I agree. We'll need at least a lot of cross-linking between them since they are related. I think there's another question and that is loading data that is not from database into the schema. Since, as far as I remember, for embeds load and cast is exactly the same this is generally already possible with Changeset.cast followed by Changeset.apply_changes. What should be the interaction there? It seems to me this might be a bit confusing as to what function you're supposed to use in what situation.

@narrowtux
Copy link
Contributor Author

for embeds load and cast is exactly the same this is generally already possible with Changeset.cast followed by Changeset.apply_changes. What should be the interaction there? It seems to me this might be a bit confusing as to what function you're supposed to use in what situation.

One advantage over the changeset calls is that you don't need to repeat the list of fields.

The changeset approach offers more flexibility of course.

@josevalim
Copy link
Member

I think there's another question and that is loading data that is not from database into the schema.

Good call. In those cases we may not have a repo, so putting it on a repo is a no-go. I looked at Ecto.Schema and it only has the schema definition functionality, functions that work on the schema at runtime are all in Ecto, so I propose to add it as Ecto.embedded_load and work on cross-linking them in the docs.

Can you please amend it acccordingly @narrowtux ? Thanks everyone!

@narrowtux
Copy link
Contributor Author

Okay

@narrowtux
Copy link
Contributor Author

@josevalim I moved the embedded_load/3 function to the Ecto module, and moved the test to embedded_test.exs.

@josevalim josevalim merged commit c97822b into elixir-ecto:master Apr 6, 2020
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim
Copy link
Member

It feels like Ecto.embedded_dump/3 would really complement this feature, but this is good enough for getting started. Thanks @narrowtux !

@narrowtux
Copy link
Contributor Author

Yeah, an embedded_dump makes sense! It probably also works with the Ecto.Repo.Schema.do_load function.

@narrowtux
Copy link
Contributor Author

Also, I noticed that this function doesn't load the data into recursive schemas. I'll continue to work on this a bit

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