Skip to content

Make &stringify_keys/1 private. #605

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.

Sign up for GitHub

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

Merged
joshsmith merged 1 commit into from
Jan 13, 2017
Merged

Make &stringify_keys/1 private. #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions lib/code_corps/map_utils.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@ defmodule CodeCorps.MapUtils do

def keys_to_string(map), do: stringify_keys(map)

# Intercept incoming %DateTime arguments; otherwise they will match %{}
defp stringify_keys(%DateTime{} = val), do: val
Copy link
Contributor

@joshsmith joshsmith Jan 3, 2017

Choose a reason for hiding this comment

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

I think this may be covered by L20.

Copy link
Contributor

@begedin begedin Jan 3, 2017

Choose a reason for hiding this comment

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

It's not. A module struct matches a map, so without this line here, line 13 would match a %DateTime{} and try to recursively stringify those keys as well.

Copy link
Contributor

@begedin begedin Jan 3, 2017

Choose a reason for hiding this comment

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

I'm not sure if there's an is_struct guard clause, so maybe we could instead change line 13 to when is_map(map) and not is_struct(map), but without something like that, this has to stay.

Copy link
Contributor

@joshsmith joshsmith Jan 3, 2017

Choose a reason for hiding this comment

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

@begedin the bigger point is that we should not ever (I think) be hitting a %DateTime{} – moreover, in both cases these functions simply return the value. They are the same function, strictly speaking.

Copy link
Contributor Author

@arubis arubis Jan 4, 2017

Choose a reason for hiding this comment

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

Having poked around in http://erlang.org/doc/man/erlang.html#is_atom-1, there pretty definitively isn't an is_struct and nothing jumps out at me as a good stand-in. I agree with @begedin, this needs to stay here for now.

Copy link
Contributor Author

@arubis arubis Jan 4, 2017

Choose a reason for hiding this comment

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

That said, we do not need L11 (stringify_keys(nil)) because that does get picked up by stringify_keys(val), do: val.

Copy link
Contributor

@joshsmith joshsmith Jan 4, 2017

Choose a reason for hiding this comment

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

I think my point here was misunderstood.

Doing any of the following is the same as doing fn(val), do: val:

  • fn(%DateTime{} = val), do: val
  • fn(nil = val), do: val
  • fn(RandomApp.RandomModule = val), do: val

All of these are equivalent if all we're doing is passing along val.

Copy link
Contributor

@begedin begedin Jan 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just for anyone visiting this thread in the future.

The problem is that

stringify_keys(val) when is_map(val) do

clause needs to go first, in order to match properly against a map at all times.

Our App used to transform large maps which also include DateTime structs. To elixir, a struct is a map, so the code here would try to convert DateTime struct keys recursively, which was causing issues, so we needed a DateTime clause before the map clause.

However, now our code is no longer using DateTime structs within its payloads, so it should be save to remove. We don't need the clause on line 11 anymore.

The clause on line 19 is a duplicate of line 22, so we don't need 19 either.

For discussions sake, I thought about this and I believe we could match against all structs using

defp stringify_keys(%{__struct__: _} = val), do: val

on line 11, but I don't think it's necessary for us to do that right now. I believe it's enough to just remove lines 11 and 19 and then merge as is.

Copy link
Contributor Author

@arubis arubis Jan 5, 2017

Choose a reason for hiding this comment

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

Thanks for the review, guys. @begedin I may revisit this thread for your idea on matching against structs--that's clever!

L19 actually isn't a duplicate of L22--stringify_keys and stringify_key are (somewhat non-obviously) different fns (plural v. singular).

I'll push up a commit dropping L11. LMK if there's further to be done; otherwise I'll rebase against develop, re-push, and move on.

# Goes through a list and stringifies keys of any map member
def stringify_keys(nil), do: nil
def stringify_keys(map = %{}) do
defp stringify_keys(map = %{}) do
map
|> Enum.map(fn {k, v} -> {stringify_key(k), stringify_keys(v)} end)
|> Enum.into(%{})
end
def stringify_keys([head | rest]), do: [stringify_keys(head) | stringify_keys(rest)]
defp stringify_keys([head | rest]), do: [stringify_keys(head) | stringify_keys(rest)]
# Default
def stringify_keys(not_a_map), do: not_a_map

def stringify_key(k) when is_atom(k), do: Atom.to_string(k)
def stringify_key(k), do: k
defp stringify_keys(val), do: val

defp stringify_key(k) when is_atom(k), do: Atom.to_string(k)
defp stringify_key(k), do: k
end
7 changes: 6 additions & 1 deletion test/lib/code_corps/map_utils_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
defmodule CodeCorps.MapUtilsTest do
use ExUnit.Case, async: true

import CodeCorps.MapUtils, only: [rename: 3]
import CodeCorps.MapUtils, only: [keys_to_string: 1, rename: 3]

test "&rename/3 renames old key in map to new key" do
assert %{"foo" => 2} |> rename("foo", "bar") == %{"bar" => 2}
end

test "&keys_to_string/1 stringifies any keys in map" do
assert %{:a => "one", :b => "two"} |> keys_to_string == %{"a" => "one", "b" => "two"}
assert %{} |> keys_to_string == %{}
end
end