Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new AliasOrder check #547

Merged
merged 1 commit into from Jun 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .credo.exs
Expand Up @@ -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},
Expand Down
15 changes: 15 additions & 0 deletions lib/credo/backports/enum.ex
Expand Up @@ -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
152 changes: 152 additions & 0 deletions 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
44 changes: 41 additions & 3 deletions test/credo/check/readability/alias_order_test.exs
Expand Up @@ -3,8 +3,6 @@ defmodule Credo.Check.Readability.AliasOrderTest do

@described_check Credo.Check.Readability.AliasOrder

@moduletag :to_be_implemented

#
# cases NOT raising issues
#
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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