Add diff for many associations without primary keys#144
Add diff for many associations without primary keys#144ddidwyll wants to merge 4 commits intopeek-travel:masterfrom
Conversation
|
Hello, @ddidwyll! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
|
Hello! Thank you for the PR! 🎉 It would be fantastic if you could include a test for this feature. Is that possible? |
|
Yes, of course, I'll write tests a little later. |
|
I wrote a small test that shows how this feature can work. |
|
Hello! Sorry about the delay in getting back to you. Too many GitHub notifications. I'll take a look at these changes shortly! |
| prev_child = Map.get(previous_map, key) | ||
| current_child = Map.get(current_map, key) | ||
| if primary_key_fields == [] do | ||
| from = max(length(previous), length(current)) - 1 | ||
|
|
||
| do_diff(prev_child, current_child) | ||
| end) | ||
| |> Enum.reject(&no_changes?/1) | ||
| from | ||
| |> Range.new(0) | ||
| |> Enum.map(fn i -> | ||
| prev_child = Enum.at(previous, i) | ||
| current_child = Enum.at(current, i) | ||
|
|
||
| if diffs == [] do | ||
| acc | ||
| else | ||
| [{field, diffs} | acc] | ||
| do_diff(prev_child, current_child) | ||
| end) |
There was a problem hiding this comment.
The performance of this isn't great, because Enum.at traverses the whole list each time. It would be better to zip the two lists together first and traverse the zipped list instead. The problem of course is that Enum.zip stops at the shortest list. Here's a simple implementation of a zip that zips all the way to the longest list, defaulting missing elements to nil: https://gist.github.com/nathan-cruz77/586092fa2487da2bdd5603934e7db4ad
I would recommend adding this util function somewhere and using it here.
vanvoljg
left a comment
vanvoljg
left a comment
There was a problem hiding this comment.
Looks good, and thank you for the contribution.
I'd like to support doughsay's suggestion and make a couple comments for how to update your PR with the changes he recommended.
| from = max(length(previous), length(current)) - 1 | ||
|
|
||
| do_diff(prev_child, current_child) | ||
| end) | ||
| |> Enum.reject(&no_changes?/1) | ||
| from | ||
| |> Range.new(0) | ||
| |> Enum.map(fn i -> | ||
| prev_child = Enum.at(previous, i) | ||
| current_child = Enum.at(current, i) | ||
|
|
||
| if diffs == [] do | ||
| acc | ||
| else | ||
| [{field, diffs} | acc] | ||
| do_diff(prev_child, current_child) | ||
| end) |
There was a problem hiding this comment.
Along with doughsay's suggestion, once you bring in that zip_longest function, this implementation becomes:
zip_longest(previous, current)
|> Enum.map(&do_diff(&1, &2))| end) | ||
|
|
||
| keys | ||
| |> Enum.reverse() |
There was a problem hiding this comment.
However, once you make the above change, you'll need to pull this reverse back out to the end of this whole function body, in the pipeline after Enum.reject/2.
3 participants
Hi, I actively use ecto_diff in my projects, and sometimes I miss the ability to diff on embeds_many associations without a primary key.
I propose a patch to add this feature.