diff --git a/.credo.exs b/.credo.exs index 118c31c36..38ba5d737 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 b48172019..f0992a27f 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 000000000..3519ae432 --- /dev/null +++ b/lib/credo/check/readability/alias_order.ex @@ -0,0 +1,150 @@ +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_module}, {line_no, second_module}], _) + when first_module > second_module do + trigger = Name.full(second_module -- first_module) + line = [line_no: line_no, trigger: trigger, module: second_module] + + {: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 diff --git a/test/credo/check/readability/alias_order_test.exs b/test/credo/check/readability/alias_order_test.exs index 96d50ddff..a63d11a61 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