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

Select fields option #154

Closed

Conversation

leftstanding
Copy link
Contributor

Creates new Ecto.diff() option select_fields that takes a keyword list
Returns only fields that are described by select_fields when option has args
Provides :all and field: [:all] functionality when option has args
Maintains current behavior when select_fields option is not used
Adds to documentation

@leftstanding leftstanding changed the base branch from master to diff-polymorphic-many-through September 28, 2023 19:48
{:ok, pet_diff} = EctoDiff.diff(nil, pet, select_fields: only_pet)
assert %{id: {nil, pet_id}, name: {nil, "Spot"}} == pet_diff.changes

# embeds are limited to the embeded field selection, not embeded fields selection

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Typo:

Suggested change
# embeds are limited to the embeded field selection, not embeded fields selection
# embeds are limited to the embedded field selection, not embedded fields selection


field_names
selected_fields = if_only_select_fields(all_fields, current, opts)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Due to the recursive nature of EctoDiff, we'll be selecting these specific fields regardless of the nesting. So a :pet that is under [:foo] and :pet under [:bar, :baz] will both be filtered. What if the user only wants to filter the first pet but not the second?

Not a big fan of this PR, tbh. The point of EctoDiff is to show what changed between two structs. If the caller wants to ignore certain changes, they should be able to it after the diff has already been calculated.

We could expose some helper functions that operate on an existing EctoDiff in order to do that. For example, we can define a def drop_changes/1 function which takes an EctoDiff and drops any changes to non-association and non-embedded fields. We can also a drop_assoc_changes/3 and drop_embed_changes/3 which take the EctoDiff, the name of the field, and a function that takes an EctoDiff and returns and EctoDiff. This would give us a nice API for recursively dropping changes from EctoDiffs in a more granular manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason that this is a non-default behavior that has to be explicitly selected, and configured, and that EctoDiff does not change.
If you for some reason have the data structure:

%{
  bar: %{pet: %{...}},
  baz: %{bar: %{pet: %{...}}}
}

There is no way to only select one of the bar.pet instances. You can select both, everything, or none.
However if this is not the case, then you gain the ability of not selecting to diff a pet that you don't want so long as the parent's are different.

}
```

The keyword list is a depth of 2 representation of the field and subfields for each selected field. If a field is given as a key, with subfields given as the values but is not provided as the appropriate value in a parent key it will be excluded.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The keyword list is a depth of 2 representation of the field and subfields for each selected field. If a field is given as a key, with subfields given as the values but is not provided as the appropriate value in a parent key it will be excluded.
The keyword list is a depth of 2 representations of the field and subfields for each selected field. If a field is given as a key, with subfields given as the values but is not provided as the appropriate value in a parent key it will be excluded.

@leftstanding
Copy link
Contributor Author

Close: Won't do at this time.
There's much conjecture about how this should be implemented that will take some time to resolve. Originally approached as a quick win. I will close this until some answers can be agreed upon, as a fork can accomplish what is needed with less time and ceremony.

@leftstanding leftstanding deleted the select_fields-option branch September 30, 2023 13:10
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.

3 participants