Feature: Configuration Extension Functions

[halostatue]

What do you want Credo to do?

Maintaining .credo.exs across Credo versions with custom configurations is somewhat tedious. The update of the default .credo.exs doesn’t change often, but it seems to me that since Credo uses Code.eval_file/2, it might be useful to have some functions that assist with the customization of one or more of the named configurations.

Which existing behaviour would change?

There would be additional functionality to allow for code-based modifications to the default configuration. The code that we have implemented looks like:

# We only use the "default" config for now.
{%{configs: original_configs}, _} = Code.eval_file("deps/credo/.credo.exs")

default_index = Enum.find_index(original_configs, &match?(%{name: "default"}, &1))
{original_default, configs} = List.pop_at(original_configs, default_index)

default =
  original_default
  |> append_paths.(:included, ["rel/", "apps/*/tasks/"])
  |> disable_checks.(:enabled, [Credo.Check.Design.TagTODO, Credo.Check.Design.TagFIXME])
  |> move_checks.(:disabled, :enabled, [
    # We would like these enabled, but they cause lots of issues at base level
    # for now.
    {Credo.Check.Readability.ImplTrue, false},
    {Credo.Check.Readability.SingleFunctionToBlockPipe, false},
    {Credo.Check.Readability.SinglePipe, false},
    # The following checks are under discussion for enablement. The only one
    # that I think is *likely* to be enabled is StrictModuleLayout, but we
    # should discuss the other three.
    {Credo.Check.Consistency.MultiAliasImportRequireUse, false},
    {Credo.Check.Readability.MultiAlias, false},
    {Credo.Check.Readability.SeparateAliasRequire, false},
    {Credo.Check.Readability.StrictModuleLayout, false},
    # We may also enable the following checks:
    {Credo.Check.Consistency.UnusedVariableNames, false}
  ])
  |> configure_check.(:disabled, Credo.Check.Refactor.DoubleBooleanNegation, false)

%{configs: [default | configs]}

As you can see, we have implemented these as functions inside the project .credo.exs file. There’s some checking that our functions do, but we think that these would be useful. Obviously, the real functions would need to be a bit cleaner.

[rrrene]

If I understand this correctly you would like Credo to provide an API for manipulating the config:

Code.eval_file("deps/credo/.credo.exs")
|> Credo.Config.use("default")
|> Credo.Config.include_paths(["rel/", "apps/*/tasks/"])
|> Credo.Config.disable_checks([Credo.Check.Design.TagTODO, Credo.Check.Design.TagFIXME])
|> Credo.Config.enable_checks([
  {Credo.Check.Readability.SeparateAliasRequire, []},
  {Credo.Check.Readability.StrictModuleLayout, []},
  {Credo.Check.Consistency.UnusedVariableNames, []}
])

Would something like this achieve this goal?

[halostatue]

Yes. I refactored things into a module for ourselves so that the file looks like this:

import Kinetic.CredoConfig

expect_credo("1.6.6")

with_config_file(".credo.original.exs", fn configs ->
  configs
  |> update_config("default", fn config ->
    config
    |> append_paths(:included, ["rel/", "apps/*/tasks/"])
    |> disable_checks(:enabled, [Credo.Check.Design.TagTODO, Credo.Check.Design.TagFIXME])
    |> move_checks(:disabled, :enabled, [
      # We would like these enabled, but they cause lots of issues at base level
      # for now.
      {Credo.Check.Readability.ImplTrue, false},
      {Credo.Check.Readability.SingleFunctionToBlockPipe, false},
      {Credo.Check.Readability.SinglePipe, false},
      # The following checks are under discussion for enablement. The only one
      # that I think is *likely* to be enabled is StrictModuleLayout, but we
      # should discuss the other three.
      {Credo.Check.Consistency.MultiAliasImportRequireUse, false},
      {Credo.Check.Readability.MultiAlias, false},
      {Credo.Check.Readability.SeparateAliasRequire, false},
      {Credo.Check.Readability.StrictModuleLayout, false},
      # We may also enable the following checks:
      {Credo.Check.Consistency.UnusedVariableNames, false}
    ])
    |> configure_check(:disabled, Credo.Check.Refactor.DoubleBooleanNegation, false)
    |> configure_check(:enabled, Credo.Check.Design.AliasUsage,
      priority: :low,
      if_nested_deeper_than: 2,
      if_called_more_often_than: 1
    )
  end)
end)

Note that our disable_checks call actually calls configure_check for each check and sets it to false. It would be easy enough to change that to move between :enabled and :disabled instead.

Here’s our implementation, working from the outside:

defmodule Kinetic.CredoConfig do
  @moduledoc """
  A helper module to manage Credo configuration. This should *only* be used in `.credo.exs`.
  """

  def expect_credo(expected_version) do
    if !String.starts_with?(Credo.version(), expected_version) do
      Mix.shell().error("Expected Credo version #{expected_version}, got #{Credo.version()}")
    end
  end

  @doc """
  Loads the specified configuration file (using `Code.eval_file/2`). Extracts
  the `configs` list and passes it to the provided function, which *must*
  return the modified `configs` list.
  """
  def with_config_file(name \\ "deps/credo/.credo.exs", fun)
      when is_function(fun, 1) do
    {%{configs: original_configs}, _} = Code.eval_file(name)
    %{configs: fun.(original_configs)}
  end

  @doc """
  Updates the named configuration in the list of configs provided using the
  provided function.
  """
  def update_config(configs, name, fun) when is_function(fun, 1) do
    index = Enum.find_index(configs, &(&1.name == name))

    unless index do
      Mix.raise("Cannot find configuration #{name}.")
    end

    List.update_at(configs, index, fun)
  end

  def append_paths(config, type, paths) when type in [:included, :excluded] do
    update_in(config, [:files, type], &(&1 ++ List.wrap(paths)))
  end

  def configure_check(input, type, check, value) when type in [:enabled, :disabled] do
    update_in(input, [:checks, type], &Keyword.replace(&1, check, value))
  end

  def disable_checks(input, type, checks) when type in [:enabled, :disabled] do
    checks
    |> List.wrap()
    |> Enum.reduce(input, fn check, config ->
      configure_check(config, type, check, false)
    end)
  end

  def move_check(config, from, to, check)
      when from in [:enabled, :disabled] and to in [:enabled, :disabled] and
             from != to do
    {check, new_value} =
      case check do
        {_, _} -> check
        check when is_atom(check) -> {check, nil}
      end

    value =
      with {:ok, check_list} <- Map.fetch(config.checks, from),
           {:ok, old_value} <- Keyword.fetch(check_list, check) do
        if is_nil(new_value) do
          old_value
        else
          new_value
        end
      else
        _ -> Mix.raise("Cannot find check #{check} in config.checks[#{from}].")
      end

    config
    |> update_in([:checks, from], &Keyword.delete(&1, check))
    |> update_in([:checks, to], &Keyword.put(&1, check, value))
  end

  def move_checks(input, from, to, checks)
      when from in [:enabled, :disabled] and to in [:enabled, :disabled] and
             from != to do
    checks
    |> List.wrap()
    |> Enum.reduce(input, fn check, config -> move_check(config, from, to, check) end)
  end
end