ueberauth / ueberauth_identity

A username/password Strategy for Überauth
MIT License
79 stars 21 forks source link

add ability to pass a list in param_nesting #18

Closed mikereinmiller closed 7 years ago

mikereinmiller commented 7 years ago

This allows a list to be passed instead of just a string for a nested data structure. One example for needing this is to comply with the jsonapi specs. See #17

Any feedback would be appreciated, I'm sure there are some ways this could be cleaned up a bit. Thanks!

mikereinmiller commented 7 years ago

I suppose instead of adding an additional param_for that checks when is_list. It could just "force" the original string into a list and call Kernel.get_in instead of Map.get? Thoughts?

doomspork commented 7 years ago

Thank you @mikereinmiller! I'll take a peek at this today and let you know if I have any feedback, could you go ahead and add a test for this change?

mikereinmiller commented 7 years ago

@doomspork Good call on the test! Let me know any thoughts and feedback you have and I'll start getting a test added for "identity_with_nested_options". Thanks!

mikereinmiller commented 7 years ago

@doomspork The other way I was thinking is we could combine the param_for function. Something like this, if you prefer? I am feeling like this would be a cleaner approach. Also test should be up in a few.

defp param_for(conn, name, nesting) do
  attrs = nesting
  |> List.wrap
  |> Enum.map(fn(element) -> to_string(element) end)

  case Kernel.get_in(conn.params, attrs) do
    nil -> nil
    nested ->
      nested
      |> Map.get(to_string(option(conn, name)))
      |> scrub_param(option(conn, :scrub_params))
  end
end
doomspork commented 7 years ago

@mikereinmiller combining them sounds good to me 👍

mikereinmiller commented 7 years ago

@doomspork Check out the update, I cleaned up the param_for functions. I think this is a much cleaner/simpler approach. Using this will allow the nested attributes to be passed as a single string like before or as a list for nested response structures.

doomspork commented 7 years ago

Thank you @mikereinmiller! I'll look at this today or tomorrow 👍

mikereinmiller commented 7 years ago

Hey @doomspork I'm sure you are pretty busy, but I was just curious if you had a chance to look at this? If there are any issues with it, just let me know and we can get it sorted out. Thanks!

doomspork commented 7 years ago

Hey @mikereinmiller! Sorry bud, I've been swamped with some work deadlines and sick family, I promise to get to this ASAP.

mikereinmiller commented 7 years ago

@doomspork No worries, I understand, stuff comes up. I really appreciate it.

doomspork commented 7 years ago

Thanks again for your contribution @mikereinmiller 😀