Support Ecto Embed

[maennchen]

Replaces the functionality of embeds in #21, #30 and #60

For associations there's an easy point to hook in in the Serializer as well.

• Merge Refactor Duplicated Code #124 first

[izelnakri]

Hi @maennchen , thank you for your contribution! This seems to be a useful feature particularly because it makes db constraints easier to define on flat attributes/keys instead of having such keys inside the %Version{}.meta Map.

We should also document this feature well. Could you add the documentation to the README.md, also would be helpful to add a description to this PR, and share an example where you needed this feature.

This PR will likely get merged, there is also one more PR in the pipeline that I need to review/merge first: #83 . So we might need to rebase and fix merge conflicts afterwards, fyi.

[maennchen]

@izelnakri I'm not sure what exactly you want to document. Essentially "supports ecto embeds" says everything already.

An example can be found in the tests, I can't link the place where we use is ourselves since the source is closed at the moment.

[izelnakri]

@maennchen If I understood correctly we could achieve what ecto embed does by storing the embedded model as json without types(types in elixir are not a first-class citizen anyways), not sure I would like to add this runtime complexity to the library. Could you give me some real-life examples where embed is useful?

[maennchen]

@izelnakri embed is useful to have Ecto.Changeset to validate your data / build forms.

I hope you accept this change because this is required to have full ecto support.

[izelnakri]

Thanks for your quick response. I think we at least need a test case which could demonstrate to me the usefullness of this feature fully. I checked the ecto documentation, after several years, it generally touches on advanced topics however certain parts are still missing for me to grasp all the features library does, and how they work in details. Nonetheless our aim is to test everything well and full so test cases will need to be written for this feature.

[maennchen]

@izelnakri I've basically rewritten this PR since rebasing was not feasable.

The tests you're looking for should be here:

• https://github.com/izelnakri/paper_trail/pull/112/files#diff-6fc8a204da55df7353b15220afa17117be281e40849223a82a892acd9f248f10R347
• https://github.com/izelnakri/paper_trail/pull/112/files#diff-6fc8a204da55df7353b15220afa17117be281e40849223a82a892acd9f248f10R453
• https://github.com/izelnakri/paper_trail/pull/112/files#diff-6fc8a204da55df7353b15220afa17117be281e40849223a82a892acd9f248f10R565

[maennchen]

@izelnakri Look at the changes commit by commit btw. It makes more sense that way. If you prefer I can also split it up into multiple PRs.

[maennchen]

@izelnakri Any update on this?

[izelnakri]

Im super busy currently, and these changes are complex/requires thoughtful reviews, Im sorry. I'll do another review this or next week.

[izelnakri]

I'm currently trying to understand this line. Could you explain this further @maennchen , are these the only cases the code can ever hit? is there a _ -> _ case as well that allows certain keys to be ignored? or do they get ignored during embed_keys or maps with normal fields hit this %Ecto.Embedded{cardinality: :one} -> {key, serialize_model_changes(value)} per each/every key? Generally its a nice PR, but would have helped if we also published the changes in few PRs as I see few changes are bit unrelated. Regardless Im currently reading up on everything, thanks for the very interesting work! We should document this functionality in the README.md as well before merging anything.

[maennchen]

There's only two cases: embeds_one and embeds_many. Therefore no catch-all is needed.

[maennchen]

There's four commits in this PR on purpose. I can open them in as many PRs as you like. (The order is kind of important though.)

[maennchen]

About documentation I don't think there's much to say. The library supports ectos features. The features are already documented at ecto.

What would you like me to mention in the README?

[maennchen]

@izelnakri I split the PR apart into #122, #123, #124

[izelnakri]

Thank you! Merged the first two, I'll also merge the third one. Could you make github clean the diff for the embed changes afterwards? I'll try to review in the next 10 days. Thanks again for all the work!

[maennchen]

@izelnakri Sure, I'll rebase it as well.

[maennchen]

@izelnakri Rebased as well.

[izelnakri]

Fantastic implementation, took me a lot of time to figure it out/fully comprehend completely but loved it :) Also definitely an improvement upon the previous change ;) Merging and cutting a release now, well done!