Refactor Ecto.Strategy to use Ecto.Schema reflections #248
Conversation
`Ecto.Schema` has a `__schema__/1` function that allows us to instrospect which fields are non-virtual, which are embeds, and which are associations. We already make use of this `__schema__` function for determining associations and embeds, but the code seemed somewhat complicated in that when we cast fields, we had to know to skip associations and embeds. By using `__schema__(:fields)` and `__schema__(:embeds)` we are able to cast fields first, then embeds, and finally associations. This has the advantage of allowing us to untangle associations and embeds code which, although is somewhat similar, does not overlap completely (there are more conditions to check when casting associations).
germsvel
force-pushed
the
gv-use-ecto-schema-reflection
branch
from
2457adc to
40cbaf8
Compare
lib/ex_machina/ecto_strategy.ex
Outdated
| end | ||
| defp cast_embed(embed, embed_key, %{__struct__: schema}) do | ||
| case embed do | ||
| %{} -> |
There was a problem hiding this comment.
Maybe this could be simplified using an if
if embed
# do stuff
endThere was a problem hiding this comment.
👍 updated in b2ec4ca
lib/ex_machina/ecto_strategy.ex
Outdated
| defp get_schema_assocs(schema) do | ||
| schema.__schema__(:associations) ++ schema.__schema__(:embeds) | ||
| defp schema_fields(schema) do | ||
| schema_non_virtual_fields(schema) -- schema_embeds(schema) |
There was a problem hiding this comment.
extra space here before --
There was a problem hiding this comment.
Nice catch. Updated in b2ec4ca
lib/ex_machina/ecto_strategy.ex
Outdated
| assoc_keys = schema_associations(schema) | ||
|
|
||
| Enum.reduce(assoc_keys, struct, fn(assoc_key, struct) -> | ||
| casted_value = Map.get(struct, assoc_key) |> cast_assoc(assoc_key, struct) |
There was a problem hiding this comment.
I see this in a couple places, but generally I think the accepted style is to use pipe chains when you can,
like struct |> Map.get(assoc_key) |> cast_assoc(assoc_key, struct)
There was a problem hiding this comment.
This is a good catch. I tend to do this, but I think the preferred style is to have the first argument and then |>. I've updated this and the other mentions in b2ec4ca
lib/ex_machina/ecto_strategy.ex
Outdated
| defp cast_all_fields(%{__struct__: schema} = struct) do | ||
| field_keys = schema_fields(schema) | ||
|
|
||
| Enum.reduce(field_keys, struct, fn(field_key, struct) -> |
There was a problem hiding this comment.
it doesn't look like you use field_keys anywhere else, so stylistically you could do this:
schema
|> schema_fields
|> Enum.reduce(struct, fn(field_key, struct) ->
...
lib/ex_machina/ecto_strategy.ex
Outdated
|
|
||
| Enum.reduce(assocs, struct, fn(assoc, struct) -> | ||
| casted_value = struct |> Map.get(assoc) |> cast_assoc(assoc, struct) | ||
| Enum.reduce(embed_keys, struct, fn(embed_key, struct) -> |
There was a problem hiding this comment.
Same here about using a pipe chain
schema
|> schema_embeds
|> Enum.reduce(struct, fn(embed_key, struct) ->
...
|
LGTM 👍 just have style comments. |
|
Thanks @paulcsmith and @dbernheisel! |
Summary
Ecto.Schemahas a__schema__/1function that allows us toinstrospect which fields are non-virtual, which are embeds, and which
are associations.
We already make use of this
__schema__function for determiningassociations and embeds, but the code seemed somewhat complicated in
that when we cast fields, we had to know to skip associations and
embeds by checking each fields type. By using
__schema__(:fields)and__schema__(:embeds)we are able to cast fields first, then embeds, and finally associations.This has the advantage of allowing us to untangle associations and
embeds code which, although is somewhat similar, does not overlap
completely (there are more conditions to check when casting
associations).