-
Notifications
You must be signed in to change notification settings - Fork 423
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
Ignore PredicateFunctionNames when is impl #1121
Ignore PredicateFunctionNames when is impl #1121
Conversation
I like this in principle and will add suggestions later this week. 🎉 |
|> run_check(@described_check) | ||
|> refute_issues() | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed change yields a couple of false negatives which we should iron out before I review this PR:
test "it should report a violation with false negatives" do | |
""" | |
defmodule FooImpl do | |
def impl(false), do: false | |
def impl(true), do: true | |
def is_bar do | |
end | |
end | |
""" | |
|> to_source_file | |
|> run_check(@described_check) | |
|> assert_issue() | |
end | |
test "it should report a violation with false negatives /2" do | |
""" | |
defmodule FooImpl do | |
impl(true) | |
def is_bar do | |
end | |
end | |
""" | |
|> to_source_file | |
|> run_check(@described_check) | |
|> assert_issue() | |
end | |
test "it should report a violation with false negatives /3" do | |
""" | |
defmodule Foo do | |
@impl is_bar(a) | |
end | |
defmodule FooImpl do | |
def is_bar do | |
end | |
end | |
""" | |
|> to_source_file | |
|> run_check(@described_check) | |
|> assert_issue() | |
end | |
test "it should report a violation with false negatives /4" do | |
""" | |
defmodule Foo do | |
@impl true | |
end | |
defmodule FooImpl do | |
def is_bar do | |
end | |
end | |
""" | |
|> to_source_file | |
|> run_check(@described_check) | |
|> assert_issue() | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I took another look and came up with some changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more nitpicks, than we should be ready to merge 👍
end | ||
|
||
defp do_find_impls_in_block({:@, _, [{:impl, _, [impl]}]}, acc) when impl != false do | ||
[:impl | acc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this atom to something more descriptive, like :record_next_definition
, so that it tells what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
impl_list = Credo.Code.prewalk(source_file, &find_impls(&1, &2)) | ||
|
||
Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta, impl_list)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This traverses the whole file for @impl
even if there is no need.
I would love for this logic to be reversed:
issue_candidates = Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
if issue_candidates == [] do
[]
else
impl_list = Credo.Code.prewalk(source_file, &find_impls(&1, &2))
# and then reject the issue_candidates that have an `@impl`
end
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored
@quangngd Thx! ✌️ |
Overview
Impl details
If any
:def, :defp, :defmacro
is preceded with:impl
, we ignore.Code is mostly inspired from the impl of
Specs