Skip to content

Commit

Permalink
Implement NegatedIsNil check.
Browse files Browse the repository at this point in the history
  • Loading branch information
pdgonzalez872 committed Jan 28, 2020
1 parent eb59a8b commit bd43c7b
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 0 deletions.
54 changes: 54 additions & 0 deletions lib/credo/check/refactor/negated_is_nil.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
defmodule Credo.Check.Refactor.NegatedIsNil do
@moduledoc false

@checkdoc """
We should avoid negating the `is_nil` predicate function.
Here are a couple of examples:
The code here ...
def fun(%{external_id: external_id, id: id}) when not is_nil(external_id) do
...
end
... can be refactored to look like this:
def fun(%{external_id: nil, id: id}) do
...
end
def fun(%{external_id: external_id, id: id}) do
...
end
Similar to negating unless blocks, the reason for this check is not
technical, but a human one. If we can use the positive, more direct and human
friendly case, we should.
"""
@explanation [check: @checkdoc]

use Credo.Check, base_priority: :low

@doc false
def run(source_file, params \\ []) do
issue_meta = IssueMeta.for(source_file, params)

Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
end

defp traverse({:when, meta, [_, {negation, _, [{:is_nil, _, _}]}]} = ast, issues, issue_meta)
when negation in [:!, :not] do
issue =
format_issue(
issue_meta,
message: "Negated is_nil in guard clause found",
trigger: "when !/not is_nil",
line_no: meta[:line]
)

{ast, [issue | issues]}
end

defp traverse(ast, issues, _), do: {ast, issues}
end
56 changes: 56 additions & 0 deletions test/credo/check/refactor/negated_is_nil_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
defmodule Credo.Check.Refactor.NegatedIsNilTest do
use Credo.TestHelper

@described_check Credo.Check.Refactor.NegatedIsNil

#
# cases NOT raising issues
#

test "it should NOT report expected code" do
"""
defmodule CredoSampleModule do
def some_function(parameter1, nil) do
something
end
def some_function(parameter1, parameter2) do
something
end
# `is_nil` in guard still works
def common_guard(%{a: a, b: b}) when is_nil(b) do
something
end
end
"""
|> to_source_file()
|> refute_issues(@described_check)
end

#
# cases raising issues
#

test "it should report a violation - `when not is_nil`" do
"""
defmodule CredoSampleModule do
def some_function(parameter1, parameter2) when not is_nil(parameter2) do
something
end
end
"""
|> to_source_file()
|> assert_issue(@described_check)
end

test "it should report a violation - `when !is_nil`" do
"""
defmodule CredoSampleModule do
def some_function(parameter1, parameter2) when !is_nil(parameter2) do
something
end
end
"""
|> to_source_file()
|> assert_issue(@described_check)
end
end

0 comments on commit bd43c7b

Please sign in to comment.