Added keys_to_atoms option for display maps

[ajardolino3]

[WIP] We use lots of maps with string keys instead of atoms because we have a lot of dynamic data. I wanted to be able to view these maps using your library for debugging purposes, but its annoying to see string keys as [0] [1] pairs. So I added an option keys_to_atoms which when set to true, automatically converts the string keys to atoms. Would never use this in production code, because atoms would take up memory, but great for testing.

Only problem is, I could not figure out how to modify the code to allow this option to be set in the config, similar to how the numbers option works. So could use some help/guidance on best way to do that with your code.

Thanks and I look forward to hearing from you.

[BjRo]

Hey there,

this is the first batch of review findings. I have to leave now, but I thought I might already post some feedback back.

I'll add the part regarding the configuration later

[BjRo]

So you can directly pattern match in the function head of convert_keys_to_atoms on the settings and spare the keys_to_atoms? function. I would also probably strip of the convert_ prefix.

def keys_to_atoms(data, shouldConvert)
def keys_to_atoms(data, true) do
  #rest of the impl
end
def keys_to_atoms(data, _), do: data

[BjRo]

Please reformat this as a pipe

data
  |> keys_to_atoms(options[:keys_to_atoms])
  |> Map.to_list
  |> Apex.Format.Seq(
       options,
       start_token: "\%{",
       end_token: "}",
       numbers: false)
  |> colorize(data, options)

[BjRo]

The change above should simplify the body of the function to

def keys_to_atoms(data, true) do
   Map.new(data, fn {k,v} -> {convert_to_atom(k), v} end)
end

Please also take into account that the code should also be able to handle non atom and non string keys. Currently the code would try to invoke String.to_atom also on other types and would blow up.

def convert_to_atom(key)
def convert_to_atom(key) when is_binary(key), do: String.to_atom(key)
def convert_to_atom(key), do: key

[BjRo]

Regarding the configuration question, if I'm not mistaken numbers can't be currently set via application config (since the only place I'm reading from it is in Colors https://github.com/BjRo/apex/search?utf8=%E2%9C%93&q=Application.get_env&type=

The way I would approach this is by reading the apex configuration section here merge it with the user supplied configuration and pass it down.

  def ap(data, options \\ []) do
    ap_options = :apex
                            |> Application.get_env([])
                            |> Keyword.merge(options)
    
    formatted = Apex.Format.format(data, ap_options)
    IO.puts(formatted)
    data
  end

Something like this. This would then also work for numbers. So you could set

use Mix.Config

config :apex, numbers: false, keys_to_atoms: true

I guess that would work. Does this help?

[ajardolino3]

Thanks for your suggestions. Very good. I will make your suggested changes and resubmit the pull request. Art

…

On Sat, Apr 1, 2017 at 10:19 AM Björn Rochel ***@***.***> wrote: Regarding the configuration question, if I'm not mistaken numbers can't be currently set via application config (since the only place I'm reading from it is in Colors https://github.com/BjRo/apex/search?utf8=%E2%9C%93&q=Application.get_env&type= The way I would approach this is by reading the apex configuration section here <https://github.com/BjRo/apex/blob/1e7ac2bca194b47ddd31c61e3ccaf8e667a8374c/lib/apex.ex#L7-L10> merge it with the user supplied configuration and pass it down. def ap(data, options \\ []) do ap_options = :apex |> Application.get_env([]) |> Keyword.merge(options) formatted = Apex.Format.format(data, ap_options) IO.puts(formatted) data end Something like this. This would then also work for numbers. So you could set use Mix.Config config :apex, numbers: false, keys_to_atoms: true I guess that would work. Does this help? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#28 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYnN3pqcEByZ1d-4hQATBNJyaVll63Q_ks5rrmsAgaJpZM4MuoLr> .

[ajardolino3]

Suggested changes were made. Now keys_to_atoms works with config. Ready for review!

[BjRo]

Only minor nitpicks. Other then that looks good 👍

[BjRo]

I would probably not do the config reading here. Doing it here would only apply it for the Map protocol implementation.

A better position would be in ap before the protocol invocation here.

Then I would also probably extract it into defp configuration(options).

[BjRo]

👍

[BjRo]

There's inconsistent whitespace usage here before convert_to_atom:

fn {k,v} -> { convert_to_atom(k), v} end

Please remove the whitespace for the sake of consistency

[BjRo]

also only a small nitpick. Please use "snake_case" for parameter names, so should_convert here.