Repository: rrrene/credo
Pull request #547 (2018-06-03 16:11:58)

Add new AliasOrder check
There are already [tests](https://github.com/rrrene/credo/blob/master/test/credo/check/readability/alias_order_test.exs) for this check, waiting to be implemented. They now all pass.

@rrrene This is my first time implementing a check as complex as this one, so I’m looking for feedback if I should have done things differently.

Thank you! 😊 

rrrene: @remiprev :+1: that looks pretty good already!

One thing I overlooked in the tests is something like

```elixir
      alias Credo.CLI.{Sorter,Filename,Options}
```

Could you add those to the tests and ensure that Multi-Alias is ordered as well?
remiprev: What would be the behavior in that case? It should raise an error saying that "Sorter" is not ordered correctly? So the following line should not raise errors, is that correct?

``` 
alias Credo.CLI.{Filename,Options,Sorter}
```
remiprev: @rrrene Done! 🎉 I added two new tests and made them pass.

Patch:
diff --git a/.credo.exs b/.credo.exs
index 118c31c3..38ba5d73 100644
--- a/.credo.exs
+++ b/.credo.exs
@@ -82,6 +82,7 @@
         #
         ## Readability Checks
         #
+        {Credo.Check.Readability.AliasOrder},
         {Credo.Check.Readability.FunctionNames},
         {Credo.Check.Readability.LargeNumbers},
         {Credo.Check.Readability.MaxLineLength, priority: :low, max_length: 80},
diff --git a/lib/credo/backports/enum.ex b/lib/credo/backports/enum.ex
index b4817201..f0992a27 100644
--- a/lib/credo/backports/enum.ex
+++ b/lib/credo/backports/enum.ex
@@ -21,4 +21,19 @@ defmodule Credo.Backports.Enum do
   else
     def split_with(list, fun), do: Enum.partition(list, fun)
   end
+
+  @doc """
+  Splits the enumerable in two lists according to the given function fun.
+
+      iex> Credo.Backports.Enum.chunk_every([1, 2, 3, 4, 5, 6], 2, 1)
+      [[1, 2], [2, 3], [3, 4], [4, 5], [5, 6], [6]]
+
+  """
+  def chunk_every(list, count, step)
+
+  if Version.match?(System.version(), ">= 1.5.0-rc") do
+    def chunk_every(list, count, step), do: Enum.chunk_every(list, count, step)
+  else
+    def chunk_every(list, count, step), do: Enum.chunk(list, count, step, [])
+  end
 end
diff --git a/lib/credo/check/readability/alias_order.ex b/lib/credo/check/readability/alias_order.ex
new file mode 100644
index 00000000..97253490
--- /dev/null
+++ b/lib/credo/check/readability/alias_order.ex
@@ -0,0 +1,152 @@
+defmodule Credo.Check.Readability.AliasOrder do
+  @moduledoc """
+  Alphabetically ordered lists are more easily scannable by the read.
+
+      # preferred
+
+      alias Module1
+      alias Module2
+      alias Module3
+
+      # NOT preferred
+
+      alias Module1
+      alias Module3
+      alias Module2
+
+  Alias should be alphabetically ordered among their group:
+
+      # preferred
+
+      alias Module3
+      alias Module4
+
+      alias Module1
+      alias Module2
+
+      # NOT preferred
+
+      alias Module3
+      alias Module4
+
+      alias Module2
+      alias Module1
+
+  Like all `Readability` issues, this one is not a technical concern.
+  But you can improve the odds of others reading and liking your code by making
+  it easier to follow.
+  """
+
+  @explanation [check: @moduledoc]
+
+  alias Credo.Code
+  alias Credo.Code.Name
+
+  use Credo.Check, base_priority: :high
+
+  @doc false
+  def run(source_file, params \\ []) do
+    issue_meta = IssueMeta.for(source_file, params)
+
+    Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+  end
+
+  defp traverse({:defmodule, _, _} = ast, issues, issue_meta) do
+    new_issues =
+      ast
+      |> extract_alias_groups()
+      |> Enum.reduce([], &traverse_groups(&1, &2, issue_meta))
+
+    {ast, issues ++ new_issues}
+  end
+
+  defp traverse(ast, issues, _), do: {ast, issues}
+
+  defp traverse_groups(group, acc, issue_meta) do
+    group
+    |> Credo.Backports.Enum.chunk_every(2, 1)
+    |> Enum.reduce_while(nil, &process_group/2)
+    |> case do
+      nil ->
+        acc
+
+      line ->
+        acc ++ [issue_for(issue_meta, line)]
+    end
+  end
+
+  defp process_group([{_, first}, {line_no, second}], _) when first > second do
+    line = [
+      line_no: line_no,
+      trigger: Name.full(second -- first),
+      module: second
+    ]
+
+    {:halt, line}
+  end
+
+  defp process_group(_, _), do: {:cont, nil}
+
+  defp extract_alias_groups({:defmodule, _, _} = ast) do
+    ast
+    |> Code.postwalk(&find_alias_groups/2)
+    |> Enum.reverse()
+    |> Enum.reduce([[]], fn definition, acc ->
+      case definition do
+        nil ->
+          [[]] ++ acc
+
+        definition ->
+          [group | groups] = acc
+          [group ++ [definition]] ++ groups
+      end
+    end)
+    |> Enum.reverse()
+  end
+
+  defp find_alias_groups(
+         {:alias, _, [{:__aliases__, meta, mod_list}]} = ast,
+         aliases
+       ) do
+    modules =
+      mod_list
+      |> (&[{meta[:line], &1}]).()
+
+    accumulate_alias_into_group(ast, modules, meta[:line], aliases)
+  end
+
+  defp find_alias_groups(
+         {:alias, _,
+          [
+            {{:., _, [{:__aliases__, meta, mod_list}, :{}]}, _, multi_mod_list}
+          ]} = ast,
+         aliases
+       ) do
+    modules =
+      multi_mod_list
+      |> Enum.map(fn {:__aliases__, meta2, mod} -> {meta2[:line], mod_list ++ mod} end)
+      |> Enum.reverse()
+
+    accumulate_alias_into_group(ast, modules, meta[:line], aliases)
+  end
+
+  defp find_alias_groups(ast, aliases), do: {ast, aliases}
+
+  defp accumulate_alias_into_group(ast, modules, line, [{line_no, _} | _] = aliases)
+       when line_no != 0 and line_no != line - 1 do
+    {ast, modules ++ [nil] ++ aliases}
+  end
+
+  defp accumulate_alias_into_group(ast, modules, _, aliases) do
+    {ast, modules ++ aliases}
+  end
+
+  defp issue_for(issue_meta, line_no: line_no, trigger: trigger, module: module) do
+    format_issue(
+      issue_meta,
+      message: "The alias `#{Name.full(module)}` is not alphabetically ordered among its group.",
+      trigger: trigger,
+      line_no: line_no
+    )
+  end
+end


Test patch:
diff --git a/test/credo/check/readability/alias_order_test.exs b/test/credo/check/readability/alias_order_test.exs
index 96d50ddf..a63d11a6 100644
--- a/test/credo/check/readability/alias_order_test.exs
+++ b/test/credo/check/readability/alias_order_test.exs
@@ -3,8 +3,6 @@ defmodule Credo.Check.Readability.AliasOrderTest do
 
   @described_check Credo.Check.Readability.AliasOrder
 
-  @moduletag :to_be_implemented
-
   #
   # cases NOT raising issues
   #
@@ -38,6 +36,24 @@ defmodule Credo.Check.Readability.AliasOrderTest do
     |> refute_issues(@described_check)
   end
 
+  test "it should NOT report violation for multi-aliases when they are alpha-ordered" do
+    """
+    defmodule Test do
+      alias App.CLI.{Filename,Sorter}
+      alias App.Foo.{Bar,Baz}
+
+      alias Credo.CLI.Command
+      alias Credo.CLI.Filename
+      alias Credo.CLI.Sorter
+
+      alias App.Module1
+      alias App.Module2
+    end
+    """
+    |> to_source_file
+    |> refute_issues(@described_check)
+  end
+
   test "it should work with __MODULE__" do
     """
     defmodule Test do
@@ -68,7 +84,7 @@ defmodule Credo.Check.Readability.AliasOrderTest do
     |> assert_issue(@described_check)
   end
 
-  test "it should report a violation /2" do
+  test "it should report a violation with alias groups" do
     """
     defmodule CredoSampleModule do
       alias Credo.CLI.Command
@@ -82,4 +98,26 @@ defmodule Credo.Check.Readability.AliasOrderTest do
     |> to_source_file
     |> assert_issue(@described_check)
   end
+
+  test "it should report a violation with multi-alias" do
+    """
+    defmodule CredoSampleModule do
+      alias App.CLI.{Bar,Baz}
+      alias App.Foo.{
+        Sorter,
+        Command,
+        Filename
+      }
+
+      alias Credo.CLI.Command
+      alias Credo.CLI.Filename
+      alias Credo.CLI.Sorter
+
+      alias App.Module1
+      alias App.Module2
+    end
+    """
+    |> to_source_file
+    |> assert_issue(@described_check)
+  end
 end