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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing to ignore modules on Credo.Check.Readability.AliasAs #987

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions lib/credo/check/readability/alias_as.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ defmodule Credo.Check.Readability.AliasAs do
use Credo.Check,
base_priority: :low,
tags: [:experimental],
param_defaults: [
ignore: []
],
explanations: [
check: """
Aliases which are not completely renamed using the `:as` option are easier to follow.
Expand Down Expand Up @@ -30,29 +33,52 @@ defmodule Credo.Check.Readability.AliasAs do
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.
"""
""",
params: [
ignore: "List of modules to ignore and allow to `alias Module, as: ...`"
]
]

@doc false
@impl true
def run(%SourceFile{} = source_file, params) do
ignore = Params.get(params, :ignore, __MODULE__)

source_file
|> Credo.Code.prewalk(&traverse(&1, &2, IssueMeta.for(source_file, params)))
|> Credo.Code.prewalk(&traverse(&1, &2, IssueMeta.for(source_file, params), ignore))
|> Enum.reverse()
end

defp traverse(ast, issues, issue_meta), do: {ast, add_issue(issues, issue(ast, issue_meta))}
defp traverse(ast, issues, issue_meta, ignore),
do: {ast, add_issue(issues, issue(ast, issue_meta, ignore))}

defp add_issue(issues, nil), do: issues
defp add_issue(issues, issue), do: [issue | issues]

defp issue({:alias, _, [{:__MODULE__, _, nil}, [as: {_, meta, _}]]}, issue_meta),
do: issue_for(issue_meta, meta[:line], inspect(:__MODULE__))
defp issue({:alias, _, [{:__MODULE__, _, nil}, [as: {_, meta, _}]]}, issue_meta, ignore) do
line = meta[:line]
{Credo.IssueMeta, source_file, _check_params} = issue_meta
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the functions in Credo.IssueMeta to deal with IssueMeta.

{_def, module_name} = Check.scope_for(source_file, line: line)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems expensive to get the scope for each occurrence instead of passing the module name during traversal.

module = Module.concat([module_name])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use Credo.Code.Name.full/1 instead of Module.concat/1.


if :__MODULE__ not in ignore and module not in ignore do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If :__MODULE__ is in ignore than we could have saved a lot of time calculating module.

issue_for(issue_meta, line, inspect(:__MODULE__))
else
nil
end
end

defp issue({:alias, _, [{_, _, original}, [as: {_, meta, _}]]}, issue_meta),
do: issue_for(issue_meta, meta[:line], inspect(Module.concat(original)))
defp issue({:alias, _, [{_, _, original}, [as: {_, meta, _}]]}, issue_meta, ignore) do
module = Module.concat(original)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use Credo.Code.Name.full/1 instead of Module.concat/1.


if module not in ignore do
issue_for(issue_meta, meta[:line], inspect(module))
else
nil
end
end

defp issue(_ast, _issue_meta), do: nil
defp issue(_ast, _issue_meta, _ignore), do: nil

defp issue_for(issue_meta, line_no, trigger) do
format_issue(
Expand Down
33 changes: 33 additions & 0 deletions test/credo/check/readability/alias_as_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,39 @@ defmodule Credo.Check.Readability.AliasAsTest do
assert issue3.trigger == "App.Module4"
end

test "it should ignore violations for ignored modules" do
"""
defmodule Test do
alias App.Module1, as: M1
end
"""
|> to_source_file
|> run_check(@described_check, ignore: [App.Module1])
|> refute_issues()
end

test "it should ignore violations for __MODULE__ when :__MODULE__ is in ignore list" do
"""
defmodule Test do
alias __MODULE__, as: Foo
end
"""
|> to_source_file
|> run_check(@described_check, ignore: [:__MODULE__])
|> refute_issues()
end

test "it should ignore violations for __MODULE__ when the module is in ignore list" do
"""
defmodule Test do
alias __MODULE__, as: Foo
end
"""
|> to_source_file
|> run_check(@described_check, ignore: [Test])
|> refute_issues()
end

test "it should not raise on alias __MODULE__, as: Foo" do
_ =
"""
Expand Down