Refactor Some Code #205
Refactor Some Code #205
Conversation
There was a problem hiding this comment.
Thanks a ton. Nice simple refactors that help clean things up 👍 Just one thing that would be great if you changed. Reverting the start_link and reset functions https://github.com/thoughtbot/ex_machina/pull/205/files#r105686541
lib/ex_machina.ex
Outdated
| defp build_function_name(factory_name) do | ||
| factory_name | ||
| |> Atom.to_string | ||
| <> "_factory" |
There was a problem hiding this comment.
Could you do Kernel.<>("_factory") here instead?
There was a problem hiding this comment.
I think that's what we're doing. It's taking Atom.to_string(factory_name) <> "_factory". Can you expand more on what you mean?
There was a problem hiding this comment.
Sorry for the confusion. I meant to type |> Kernel.<>("_factory") that way you can keep the pipe chain going
There was a problem hiding this comment.
Oh, good catch. Running this code in my iex and seeing that it doesn't actually work...
There was a problem hiding this comment.
Sorry for jumping to the party and possibly silly question, but why not simply:
"#{factory_name}_factory"There was a problem hiding this comment.
@pdawczak that's a very very good question :p
| end | ||
|
|
||
| defp do_merge(%{__struct__: _} = record, attrs), do: struct!(record, attrs) | ||
| defp do_merge(record, attrs), do: Map.merge(record, attrs) |
| @@ -177,20 +180,16 @@ defmodule ExMachina do | |||
| build_list(3, :user) | |||
| """ | |||
| def build_list(module, number_of_factories, factory_name, attrs \\ %{}) do | |||
| Enum.map(1..number_of_factories, fn(_) -> | |||
| Enum.map 1..number_of_factories, fn(_) -> | |||
There was a problem hiding this comment.
Yeah that looks nicer 👍
| @doc """ | ||
| Raises a helpful error if no factory is defined. | ||
| """ | ||
| @doc "Raises a helpful error if no factory is defined." |
| @@ -230,7 +226,7 @@ defmodule ExMachina.Ecto do | |||
| defp insert_built_belongs_to_assoc(module, association, record) do | |||
| case Map.get(record, association.field) do | |||
| built_relation = %{__meta__: %{state: :built}} -> | |||
| relation = built_relation |> module.insert | |||
| relation = module.insert(built_relation) | |||
There was a problem hiding this comment.
Good idea. That reads better
| ) | ||
| Enum.reduce struct, Map.new, fn({key, value}, acc) -> | ||
| Map.put(acc, to_string(key), value) | ||
| end |
lib/ex_machina/sequence.ex
Outdated
| def reset do | ||
| Agent.update(__MODULE__, fn(_) -> Map.new end) | ||
| end | ||
| def reset, do: Agent.update(__MODULE__, fn(_) -> Map.new end) |
There was a problem hiding this comment.
For this and start_link I think I prefer the previous version. This is a bit too busy IMO
BenMorganIO
force-pushed
the
refactor-ecto
branch
from
1864647
to
81ffde1
Compare
|
@paulcsmith I've reverted and rebased the changes with |
BenMorganIO
force-pushed
the
refactor-ecto
branch
from
81ffde1
to
62c0aa2
Compare
|
Thanks a ton @BenMorganIO! |
Hey guys,
I was going through the open source code and reasoning about the implementation. I noticed there was some code that could have been refactored. I hope this helps :)