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

Attributes collection can be keyword list or map #12

Closed
wants to merge 1 commit into from

Conversation

plicjo
Copy link
Contributor

@plicjo plicjo commented Dec 3, 2020

Closes #10

@@ -46,7 +46,7 @@ defmodule EctoFactory do
"""
@spec schema(atom() | Ecto.Schema, keyword()) :: map()
def attrs(factory_name, attrs \\ []) do
build_attrs(factory_name, attrs)
build_attrs(factory_name, to_keyword_list(attrs))
|> elem(1)
|> Enum.into(%{}, fn {k, v} -> {to_string(k), v} end)
Copy link
Owner

Choose a reason for hiding this comment

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

After looking at this. We're really iterating through attrs a few times. I bet we could make it more efficient if build_attrs took a keyword or a map, and also converted the key to a string.

Wanna try that?

Copy link
Contributor Author

@plicjo plicjo Dec 3, 2020

Choose a reason for hiding this comment

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

I'm fairly certain that I don't understand what you mean.

...but I did try this to convert to string up top:

def attrs(factory_name, attrs \\ []) do
  build_attrs(factory_name, Enum.into(attrs, [], fn {k, v} -> {to_string(k), v} end))
  |> elem(1)
  |> Enum.into(%{})
end

And I got the following:

 1) test build attrs by using defined factory and passed in attributes (EctoFactoryTest)
     test/ecto_factory_test.exs:43
     ** (ArgumentError) expected a keyword list, but an entry in the list is not a two-element tuple with an atom as its first element, got: {"age", 99}
     code: user = EctoFactory.attrs(:user_with_default_username, age: 99)
     stacktrace:
       (elixir 1.11.2) lib/keyword.ex:475: Keyword.keys/1
       (ecto_factory 0.2.0) lib/ecto_factory.ex:119: EctoFactory.build_attrs/2
       (ecto_factory 0.2.0) lib/ecto_factory.ex:49: EctoFactory.attrs/2
       test/ecto_factory_test.exs:44: (test)

Some thoughts:

  1. I have no idea if I understand your request.
  2. I have no idea if Ecto wants atoms (I think it does, dunno how to confirm).

lib/ecto_factory.ex Outdated Show resolved Hide resolved
@plicjo
Copy link
Contributor Author

plicjo commented Dec 3, 2020

@mrmicahcooper How about now?

@plicjo plicjo closed this Dec 3, 2020
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.

Respond to keyword lists and maps?
2 participants