Code clean-up to satisfy Credo complaints (#69)

* Code clean-up to satisfy Credo complaints

* Add and configure Credo

.credo.exs

@@ -0,0 +1,150 @@
+# This file contains the configuration for Credo and you are probably reading
+# this after creating it with `mix credo.gen.config`.
+#
+# If you find anything wrong or unclear in this file, please report an
+# issue on GitHub: https://github.com/rrrene/credo/issues
+#
+%{
+  #
+  # You can have as many configs as you like in the `configs:` field.
+  configs: [
+    %{
+      #
+      # Run any exec using `mix credo -C <name>`. If no exec name is given
+      # "default" is used.
+      #
+      name: "default",
+      #
+      # These are the files included in the analysis:
+      files: %{
+        #
+        # You can give explicit globs or simply directories.
+        # In the latter case `**/*.{ex,exs}` will be used.
+      #
+        included: ["lib/", "src/", "web/", "apps/"],
+        excluded: [~r"/_build/", ~r"/deps/"]
+      },
+      #
+      # If you create your own checks, you must specify the source files for
+      # them here, so they can be loaded by Credo before running the analysis.
+      #
+      requires: [],
+      #
+      # Credo automatically checks for updates, like e.g. Hex does.
+      # You can disable this behaviour below:
+      #
+      check_for_updates: true,
+      #
+      # If you want to enforce a style guide and need a more traditional linting
+      # experience, you can change `strict` to `true` below:
+      #
+      strict: false,
+      #
+      # If you want to use uncolored output by default, you can change `color`
+      # to `false` below:
+      #
+      color: true,
+      #
+      # You can customize the parameters of any check by adding a second element
+      # to the tuple.
+      #
+      # To disable a check put `false` as second element:
+      #
+      #     {Credo.Check.Design.DuplicatedCode, false}
+      #
+      checks: [
+        {Credo.Check.Consistency.ExceptionNames},
+        {Credo.Check.Consistency.LineEndings},
+        {Credo.Check.Consistency.ParameterPatternMatching},
+        {Credo.Check.Consistency.SpaceAroundOperators},
+        {Credo.Check.Consistency.SpaceInParentheses},
+        {Credo.Check.Consistency.TabsOrSpaces},
+
+        # For some checks, like AliasUsage, you can only customize the priority
+        # Priority values are: `low, normal, high, higher`
+        #
+        {Credo.Check.Design.AliasUsage, priority: :low},
+
+        # For others you can set parameters
+
+        # If you don't want the `setup` and `test` macro calls in ExUnit tests
+        # or the `schema` macro in Ecto schemas to trigger DuplicatedCode, just
+        # set the `excluded_macros` parameter to `[:schema, :setup, :test]`.
+        #
+        {Credo.Check.Design.DuplicatedCode, excluded_macros: []},
+
+        # You can also customize the exit_status of each check.
+        # If you don't want TODO comments to cause `mix credo` to fail, just
+        # set this value to 0 (zero).
+        #
+        {Credo.Check.Design.TagTODO, exit_status: 0},
+        {Credo.Check.Design.TagFIXME},
+
+        {Credo.Check.Readability.FunctionNames},
+        {Credo.Check.Readability.LargeNumbers},
+        {Credo.Check.Readability.MaxLineLength, priority: :low, max_length: 120},
+        {Credo.Check.Readability.ModuleAttributeNames},
+        {Credo.Check.Readability.ModuleDoc},
+        {Credo.Check.Readability.ModuleNames},
+        {Credo.Check.Readability.ParenthesesOnZeroArityDefs},
+        {Credo.Check.Readability.ParenthesesInCondition},
+        {Credo.Check.Readability.PredicateFunctionNames},
+        {Credo.Check.Readability.PreferImplicitTry},
+        {Credo.Check.Readability.RedundantBlankLines},
+        {Credo.Check.Readability.StringSigils},
+        {Credo.Check.Readability.TrailingBlankLine},
+        {Credo.Check.Readability.TrailingWhiteSpace},
+        {Credo.Check.Readability.VariableNames},
+        {Credo.Check.Readability.Semicolons},
+        {Credo.Check.Readability.SpaceAfterCommas},
+
+        {Credo.Check.Refactor.DoubleBooleanNegation},
+        {Credo.Check.Refactor.CondStatements},
+        {Credo.Check.Refactor.CyclomaticComplexity},
+        {Credo.Check.Refactor.FunctionArity},
+        {Credo.Check.Refactor.LongQuoteBlocks},
+        {Credo.Check.Refactor.MatchInCondition},
+        {Credo.Check.Refactor.NegatedConditionsInUnless},
+        {Credo.Check.Refactor.NegatedConditionsWithElse},
+        {Credo.Check.Refactor.Nesting},
+        {Credo.Check.Refactor.PipeChainStart},
+        {Credo.Check.Refactor.UnlessWithElse},
+
+        {Credo.Check.Warning.BoolOperationOnSameValues},
+        {Credo.Check.Warning.IExPry},
+        {Credo.Check.Warning.IoInspect},
+        {Credo.Check.Warning.LazyLogging},
+        {Credo.Check.Warning.OperationOnSameValues},
+        {Credo.Check.Warning.OperationWithConstantResult},
+        {Credo.Check.Warning.UnusedEnumOperation},
+        {Credo.Check.Warning.UnusedFileOperation},
+        {Credo.Check.Warning.UnusedKeywordOperation},
+        {Credo.Check.Warning.UnusedListOperation},
+        {Credo.Check.Warning.UnusedPathOperation},
+        {Credo.Check.Warning.UnusedRegexOperation},
+        {Credo.Check.Warning.UnusedStringOperation},
+        {Credo.Check.Warning.UnusedTupleOperation},
+        {Credo.Check.Warning.RaiseInsideRescue},
+
+        # Controversial and experimental checks (opt-in, just remove `, false`)
+        #
+        {Credo.Check.Refactor.ABCSize, false},
+        {Credo.Check.Refactor.AppendSingleItem, false},
+        {Credo.Check.Refactor.VariableRebinding, false},
+        {Credo.Check.Warning.MapGetUnsafePass, false},
+        {Credo.Check.Consistency.MultiAliasImportRequireUse, false},
+
+        # Deprecated checks (these will be deleted after a grace period)
+        #
+        {Credo.Check.Readability.Specs, false},
+        {Credo.Check.Warning.NameRedeclarationByAssignment, false},
+        {Credo.Check.Warning.NameRedeclarationByCase, false},
+        {Credo.Check.Warning.NameRedeclarationByDef, false},
+        {Credo.Check.Warning.NameRedeclarationByFn, false},
+
+        # Custom checks can be created using `mix credo.gen.check`.
+        #
+      ]
+    }
+  ]
+}

.travis.yml

@@ -1,3 +1,12 @@
 language: elixir
 elixir:
   - 1.4
+otp_release:
+  - 20.0
+  - 19.3
+notifications:
+  email: false
+sudo: false
+script:
+  - mix test
+  - mix credo

lib/jsonapi.ex

@@ -1,2 +1,5 @@
 defmodule JSONAPI do
+  @moduledoc """
+  A module for working with the JSON API specification in Elixir
+  """
 end

lib/jsonapi/config.ex

@@ -1,4 +1,8 @@
 defmodule JSONAPI.Config do
+  @moduledoc """
+  Configuration struct containing JSON API information for a request
+  """
+
   defstruct data: nil,
             fields: %{},
             filter: [],

lib/jsonapi/ecto.ex

@@ -1,4 +1,8 @@
 defmodule JSONAPI.Ecto do
+  @moduledoc """
+  Helper functions for working with Ecto
+  """
+
   @doc """
   Checks to see if an associated table is Loaded.
 

lib/jsonapi/plug.ex

@@ -14,10 +14,10 @@ defmodule JSONAPI.PlugResponseContentType do
 
   def call(conn, _opts) do
     register_before_send(conn, fn(conn) ->
-      if !conn.assigns[:override_jsonapi] do
-        put_resp_content_type(conn, "application/vnd.api+json")
-      else
+      if conn.assigns[:override_jsonapi] do
         conn
+      else
+        put_resp_content_type(conn, "application/vnd.api+json")
       end
     end)
   end

lib/jsonapi/query_parser.ex

@@ -44,7 +44,6 @@ defmodule JSONAPI.QueryParser do
   You will notice the fields section is a not as easy to work with as the others and
   that is a result of Ecto not supporting high quality selects quite yet. This is a WIP.
 
-
   ## Options
     * `:view` - The JSONAPI View which is the basis for this plug.
     * `:sort` - List of atoms which define which fields can be sorted on.
@@ -70,15 +69,19 @@ defmodule JSONAPI.QueryParser do
   def parse_filter(config, map) when map_size(map) == 0, do: config
   def parse_filter(%Config{opts: opts} = config, filter) do
     opts_filter = Keyword.get(opts, :filter, [])
-    Enum.reduce(filter, config, fn({key, val}, acc) ->
-      unless Enum.any?(opts_filter, fn k -> k == key end) do
-        raise InvalidQuery, resource: config.view.type(), param: key, param_type: :filter
-      end
 
+    Enum.reduce(filter, config, fn({key, val}, acc) ->
+      check_filter_validity!(opts_filter, key, config)
       %{acc | filter: Keyword.put(acc.filter, String.to_atom(key), val)}
     end)
   end
 
+  defp check_filter_validity!(filters, key, config) do
+    unless key in filters do
+      raise InvalidQuery, resource: config.view.type(), param: key, param_type: :filter
+    end
+  end
+
   def parse_fields(config, map) when map_size(map) == 0, do: config
   def parse_fields(%Config{} = config, fields) do
     Enum.reduce(fields, config, fn ({type, value}, acc) ->
@@ -160,7 +163,7 @@ defmodule JSONAPI.QueryParser do
     keys = key |> String.split(".") |> Enum.map(&String.to_existing_atom/1)
 
     last = List.last(keys)
-    path = Enum.slice(keys, 0, Enum.count(keys)-1)
+    path = Enum.slice(keys, 0, Enum.count(keys) - 1)
 
     if member_of_tree?(keys, valid_include) do
       put_as_tree([], path, last)
@@ -189,4 +192,3 @@ defmodule JSONAPI.QueryParser do
     struct(JSONAPI.Config, opts: opts, view: view)
   end
 end
-

lib/jsonapi/serializer.ex

@@ -1,4 +1,8 @@
 defmodule JSONAPI.Serializer do
+  @moduledoc """
+  Serialize a map of data into a properly formatted JSON API response object
+  """
+
   import JSONAPI.Ecto, only: [assoc_loaded?: 1]
 
   @doc """
@@ -50,48 +54,54 @@ defmodule JSONAPI.Serializer do
         meta -> Map.put(doc, :meta, meta)
       end
 
-    # Handle all the relationships
-    Enum.map_reduce(view.relationships(), doc, fn({key, include_view}, acc) ->
+    encode_relationships(conn, doc, {view, data, query_includes, valid_includes})
+  end
 
-      rel_view = case include_view do
+  def encode_relationships(conn, doc, {view, _, _, _} = view_info) do
+    rels = view.relationships()
+    Enum.map_reduce(rels, doc, &build_relationships(conn, view_info, &1, &2))
+  end
+
+  def build_relationships(conn, {view, data, query_includes, valid_includes}, {key, include_view}, acc) do
+    rel_view =
+      case include_view do
         {view, :include} -> view
         view -> view
       end
 
-      rel_data = Map.get(data, key)
-
-      only_rel_view = get_view(rel_view)
-      # Build the relationship url
-      rel_url = view.url_for_rel(data, key, conn)
-      # Build the relationship
-      acc = put_in(acc, [:relationships, key], encode_relation(only_rel_view, rel_data, rel_url, conn))
-
-      valid_include_view =
-        case valid_includes do
-          list when is_list(list) ->
-            case Keyword.get(valid_includes, key) do
-              {view, :include} -> {view, :include}
-              view -> {view, :include}
-            end
-          {view, :include} -> {view, :include}
-          view -> {view, :include}
+    rel_data = Map.get(data, key)
+
+    only_rel_view = get_view(rel_view)
+    # Build the relationship url
+    rel_url = view.url_for_rel(data, key, conn)
+    # Build the relationship
+    acc = put_in(acc, [:relationships, key], encode_relation(only_rel_view, rel_data, rel_url, conn))
+
+    valid_include_view = include_view(valid_includes, key)
+
+    if {rel_view, :include} == valid_include_view && is_data_loaded?(rel_data) do
+      rel_query_includes =
+        if is_list(query_includes) do
+          Keyword.get(query_includes, key, [])
+        else
+          []
         end
+      #TODO Possibly only return a list of data + view, and encode it after the fact once instead of N times.
+      {rel_included, encoded_rel} = encode_data(rel_view, rel_data, conn, rel_query_includes)
+      {rel_included ++ [encoded_rel], acc}
+    else
+      {nil, acc}
+    end
+  end
 
-      if {rel_view, :include} == valid_include_view && is_data_loaded?(rel_data) do
-        rel_query_includes =
-          if is_list(query_includes) do
-            Keyword.get(query_includes, key, [])
-          else
-            []
-          end
-        #TODO Possibly only return a list of data + view, and encode it after the fact once instead of N times.
-        {rel_included, encoded_rel} = encode_data(rel_view, rel_data, conn, rel_query_includes)
-        {rel_included ++ [encoded_rel], acc}
-      else
-        {nil, acc}
-      end
-    end)
+  defp include_view(valid_includes, key) when is_list(valid_includes) do
+    case Keyword.get(valid_includes, key) do
+      {view, :include} -> {view, :include}
+      view -> {view, :include}
+    end
   end
+  defp include_view({view, :include}, _key), do: {view, :include}
+  defp include_view(view, _key), do: {view, :include}
 
   def is_data_loaded?(rel_data) do
     assoc_loaded?(rel_data) && (is_map(rel_data) || (is_list(rel_data) && !Enum.empty?(rel_data)))

lib/jsonapi/utils/include_tree.ex

@@ -1,4 +1,8 @@
 defmodule JSONAPI.Utils.IncludeTree do
+  @moduledoc """
+  Internal utility for building trees of resource relationships
+  """
+
   def put_as_tree(acc, items, val) do
     [head | tail] = Enum.reverse(items)
     build_tree(Keyword.put(acc, head, val), tail)

lib/jsonapi/view.ex

@@ -116,11 +116,11 @@ defmodule JSONAPI.View do
         "#{@namespace}/#{type()}/#{id(data)}"
       end
 
-      def url_for(data, %Plug.Conn{}=conn) when is_list(data) do
+      def url_for(data, %Plug.Conn{} = conn) when is_list(data) do
         "#{scheme(conn)}://#{host(conn)}#{@namespace}/#{type()}"
       end
 
-      def url_for(data, %Plug.Conn{}=conn) do
+      def url_for(data, %Plug.Conn{} = conn) do
         "#{scheme(conn)}://#{host(conn)}#{@namespace}/#{type()}/#{id(data)}"
       end
 

mix.exs

@@ -22,7 +22,8 @@ defmodule JSONAPI.Mixfile do
       {:plug, "~> 1.0"},
       {:ex_doc, "~> 0.7", only: :dev},
       {:earmark, ">= 0.0.0", only: :dev},
-      {:poison, "~> 3.0", only: :test}
+      {:poison, "~> 3.0", only: :test},
+      {:credo, "~> 0.8", only: [:dev, :test], runtime: false}
     ]
   end