Idiomatic elixir, use credo #54
Conversation
There was a problem hiding this comment.
Overall, great changes.
Thanks for cleaning things up! 😄
Just one small nitpick: if we're going to use yoda assignment in function arguments, it should be consistent throughout the file, e.g. https://github.com/cpjk/canary/pull/54/files#diff-3938ea297feb0d441ffe05cec9da28eaR279
Other than that, LGTM.
lib/canary/plugs.ex
Outdated
|
|
||
| defp fetch_resource(conn, opts) do | ||
| repo = Application.get_env(:canary, :repo) | ||
|
|
||
| field_name = (opts[:id_field] || "id") | ||
|
|
||
| get_map_args = %{field_name => get_resource_id(conn, opts)} | ||
| get_map_args = (for {key, val} <- get_map_args, into: %{}, do: {String.to_atom(key), val}) | ||
| get_map_args = %{String.to_atom(field_name) => get_resource_id(conn, opts)} |
| @@ -414,26 +413,31 @@ defmodule Canary.Plugs do | |||
| end | |||
| end | |||
|
|
|||
| defp handle_unauthorized(conn = %{skip_canary_handler: true}, _opts), | |||
| defp handle_unauthorized(%{skip_canary_handler: true} = conn, _opts), | |||
There was a problem hiding this comment.
I like the yoda syntax, but if we are going to use it here, it should be used throught the whole code base for consistency, e.g. https://github.com/cpjk/canary/pull/54/files#diff-3938ea297feb0d441ffe05cec9da28eaR282
|
@cpjk Addressed the other yoda cases. Also fixed the merge conflict. Should be good now |
|
Sorry for the delay! The email got caught up in my work github notifications 😄. LGTM! 🚀 |
I've been using canary in a side project of mine, and noticed that the codebase could be cleaned up a bit in terms of idiomatic elixir code.
Made several changes to simplify code, and make it more idiomatic.
As far as credo goes, disabled PipeChainStart and Specs