Skip to content

feat (FilterFilter and related): Check Map and Keyword versions too #1200

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat (FilterFilter and related): Check Map and Keyword versions too #1200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 20 additions & 5 deletions lib/credo/check/refactor/filter_filter.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule Credo.Check.Refactor.FilterFilter do
id: "EX4008",
explanations: [
check: """
One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.filter/2`.
One `Enum.filter/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.filter/2 |> Enum.filter/2`.

This should be refactored:

Expand All @@ -24,17 +24,32 @@ defmodule Credo.Check.Refactor.FilterFilter do
]

alias Credo.Check.Refactor.EnumHelpers
alias Credo.Check.Refactor.MapHelpers
alias Credo.Check.Refactor.KeywordHelpers

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

message = "One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.filter/2`"
trigger = "|>"

Credo.Code.prewalk(
source_file,
&EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :filter, :filter, __MODULE__)
Enum.flat_map(
[
{&EnumHelpers.traverse/8, "Enum"},
{&MapHelpers.traverse/8, "Map"},
{&KeywordHelpers.traverse/8, "Keyword"}
],
fn {traverse, module} ->
message =
"One `#{module}.filter/2` is more efficient than `#{module}.filter/2 |> #{module}.filter/2`"

Credo.Code.prewalk(
source_file,
fn ast, issues ->
traverse.(ast, issues, issue_meta, message, trigger, :filter, :filter, __MODULE__)
end
)
end
)
end
end
25 changes: 20 additions & 5 deletions lib/credo/check/refactor/filter_reject.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule Credo.Check.Refactor.FilterReject do
tags: [:controversial],
explanations: [
check: """
One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.reject/2`.
One `Enum.filter/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.filter/2 |> Enum.reject/2`.

This should be refactored:

Expand All @@ -25,17 +25,32 @@ defmodule Credo.Check.Refactor.FilterReject do
]

alias Credo.Check.Refactor.EnumHelpers
alias Credo.Check.Refactor.MapHelpers
alias Credo.Check.Refactor.KeywordHelpers

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

message = "One `Enum.filter/2` is more efficient than `Enum.filter/2 |> Enum.reject/2`"
trigger = "|>"

Credo.Code.prewalk(
source_file,
&EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :filter, :reject, __MODULE__)
Enum.flat_map(
[
{&EnumHelpers.traverse/8, "Enum"},
{&MapHelpers.traverse/8, "Map"},
{&KeywordHelpers.traverse/8, "Keyword"}
],
fn {traverse, module} ->
message =
"One `#{module}.filter/2` is more efficient than `#{module}.filter/2 |> #{module}.reject/2`"

Credo.Code.prewalk(
source_file,
fn ast, issues ->
traverse.(ast, issues, issue_meta, message, trigger, :filter, :reject, __MODULE__)
end
)
end
)
end
end
87 changes: 87 additions & 0 deletions lib/credo/check/refactor/keyword_helpers.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
defmodule Credo.Check.Refactor.KeywordHelpers do
def traverse(
{{:., _, [{:__aliases__, meta, [:Keyword]}, second]}, _,
[{{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _}, _]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end
Comment on lines +1 to +15
Copy link
Author

Choose a reason for hiding this comment

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

This module, as well as the MapHelpers below, is a straight copy and paste of EnumHelpers, with the module name changed.

My sense based on reading the rest of the codebase was that duplication was preferable to either

  • metaprogramming to generate a KeywordHelpers and MapHelpers module, or
  • making a single, more generic module that accept the target module (:Enum, :Map, or :Keyword) as an argument

If I'm mistaken, though, I'm happy to do that refactor.


def traverse(
{:|>, meta,
[
{{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _},
{{:., _, [{:__aliases__, _, [:Keyword]}, second]}, _, _}
]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(
{{:., meta, [{:__aliases__, _, [:Keyword]}, second]}, _,
[
{:|>, _, [_, {{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _}]},
_
]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(
{:|>, meta,
[
{:|>, _,
[
_,
{{:., _, [{:__aliases__, _, [:Keyword]}, first]}, _, _}
]},
{{:., _, [{:__aliases__, _, [:Keyword]}, second]}, _, _}
]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(ast, issues, _issue_meta, _message, _trigger, _first, _second, _module) do
{ast, issues}
end

defp issue_for(issue_meta, line_no, message, trigger, module) do
module.format_issue(
issue_meta,
message: message,
trigger: trigger,
line_no: line_no
)
end
end
87 changes: 87 additions & 0 deletions lib/credo/check/refactor/map_helpers.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
defmodule Credo.Check.Refactor.MapHelpers do
def traverse(
{{:., _, [{:__aliases__, meta, [:Map]}, second]}, _,
[{{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _}, _]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(
{:|>, meta,
[
{{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _},
{{:., _, [{:__aliases__, _, [:Map]}, second]}, _, _}
]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(
{{:., meta, [{:__aliases__, _, [:Map]}, second]}, _,
[
{:|>, _, [_, {{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _}]},
_
]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(
{:|>, meta,
[
{:|>, _,
[
_,
{{:., _, [{:__aliases__, _, [:Map]}, first]}, _, _}
]},
{{:., _, [{:__aliases__, _, [:Map]}, second]}, _, _}
]} = ast,
issues,
issue_meta,
message,
trigger,
first,
second,
module
) do
new_issue = issue_for(issue_meta, meta[:line], message, trigger, module)
{ast, issues ++ List.wrap(new_issue)}
end

def traverse(ast, issues, _issue_meta, _message, _trigger, _first, _second, _module) do
{ast, issues}
end

defp issue_for(issue_meta, line_no, message, trigger, module) do
module.format_issue(
issue_meta,
message: message,
trigger: trigger,
line_no: line_no
)
end
end
25 changes: 20 additions & 5 deletions lib/credo/check/refactor/reject_filter.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ defmodule Credo.Check.Refactor.RejectFilter do
tags: [:controversial],
explanations: [
check: """
One `Enum.filter/2` is more efficient than `Enum.reject/2 |> Enum.filter/2`.
One `Enum.filter/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.reject/2 |> Enum.filter/2`.

This should be refactored:

Expand All @@ -25,17 +25,32 @@ defmodule Credo.Check.Refactor.RejectFilter do
]

alias Credo.Check.Refactor.EnumHelpers
alias Credo.Check.Refactor.MapHelpers
alias Credo.Check.Refactor.KeywordHelpers

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

message = "One `Enum.filter/2` is more efficient than `Enum.reject/2 |> Enum.filter/2`"
trigger = "|>"

Credo.Code.prewalk(
source_file,
&EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :reject, :filter, __MODULE__)
Enum.flat_map(
[
{&EnumHelpers.traverse/8, "Enum"},
{&MapHelpers.traverse/8, "Map"},
{&KeywordHelpers.traverse/8, "Keyword"}
],
fn {traverse, module} ->
message =
"One `#{module}.reject/2` is more efficient than `#{module}.reject/2 |> #{module}.filter/2`"

Credo.Code.prewalk(
source_file,
fn ast, issues ->
traverse.(ast, issues, issue_meta, message, trigger, :reject, :filter, __MODULE__)
end
)
end
)
end
end
25 changes: 20 additions & 5 deletions lib/credo/check/refactor/reject_reject.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule Credo.Check.Refactor.RejectReject do
id: "EX4026",
explanations: [
check: """
One `Enum.reject/2` is more efficient than `Enum.reject/2 |> Enum.reject/2`.
One `Enum.reject/2` (or corresponding `Map` and `Keyword` calls) is more efficient than `Enum.reject/2 |> Enum.reject/2`.

This should be refactored:

Expand All @@ -24,17 +24,32 @@ defmodule Credo.Check.Refactor.RejectReject do
]

alias Credo.Check.Refactor.EnumHelpers
alias Credo.Check.Refactor.MapHelpers
alias Credo.Check.Refactor.KeywordHelpers

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

message = "One `Enum.reject/2` is more efficient than `Enum.reject/2 |> Enum.reject/2`"
trigger = "|>"

Credo.Code.prewalk(
source_file,
&EnumHelpers.traverse(&1, &2, issue_meta, message, trigger, :reject, :reject, __MODULE__)
Enum.flat_map(
[
{&EnumHelpers.traverse/8, "Enum"},
{&MapHelpers.traverse/8, "Map"},
{&KeywordHelpers.traverse/8, "Keyword"}
],
fn {traverse, module} ->
message =
"One `#{module}.reject/2` is more efficient than `#{module}.reject/2 |> #{module}.reject/2`"

Credo.Code.prewalk(
source_file,
fn ast, issues ->
traverse.(ast, issues, issue_meta, message, trigger, :reject, :reject, __MODULE__)
end
)
end
)
end
end
Loading