with_clauses.ex (3483B)
1 defmodule Credo.Check.Refactor.WithClauses do 2 use Credo.Check, 3 base_priority: :high, 4 explanations: [ 5 check: ~S""" 6 `with` statements are useful when you need to chain a sequence 7 of pattern matches, stopping at the first one that fails. 8 9 But sometimes, we go a little overboard with them (pun intended). 10 11 If the first or last clause in a `with` statement is not a `<-` clause, 12 it still compiles and works, but is not really utilizing what the `with` 13 macro provides and can be misleading. 14 15 with ref = make_ref(), 16 {:ok, user} <- User.create(ref), 17 :ok <- send_email(user), 18 Logger.debug("Created user: #{inspect(user)}") do 19 user 20 end 21 22 Here, both the first and last clause are actually not matching anything. 23 24 If we move them outside of the `with` (the first ones) or inside the body 25 of the `with` (the last ones), the code becomes more focused and . 26 27 This `with` should be refactored like this: 28 29 ref = make_ref() 30 31 with {:ok, user} <- User.create(ref), 32 :ok <- send_email(user) do 33 Logger.debug("Created user: #{inspect(user)}") 34 user 35 end 36 """ 37 ] 38 39 @message_first_clause_not_pattern "`with` doesn't start with a <- clause, move the non-pattern <- clauses outside of the `with`" 40 @message_last_clause_not_pattern "`with` doesn't end with a <- clause, move the non-pattern <- clauses inside the body of the `with`" 41 42 @doc false 43 @impl true 44 def run(%SourceFile{} = source_file, params) do 45 issue_meta = IssueMeta.for(source_file, params) 46 Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta)) 47 end 48 49 # TODO: consider for experimental check front-loader (ast) 50 defp traverse({:with, meta, [_, _ | _] = clauses_and_body} = ast, issues, issue_meta) 51 when is_list(clauses_and_body) do 52 # If clauses_and_body is a list with at least two elements in it, we think 53 # this might be a call to the special form "with". To be sure of that, 54 # we get the last element of clauses_and_body and check that it's a keyword 55 # list with a :do key in it (the body). 56 57 # We can hard-match on [maybe_body] here since we know that clauses_and_body 58 # has at least two elements. 59 {maybe_clauses, [maybe_body]} = Enum.split(clauses_and_body, -1) 60 61 if Keyword.keyword?(maybe_body) and Keyword.has_key?(maybe_body, :do) do 62 {ast, issues_for_with(maybe_clauses, meta[:line], issue_meta) ++ issues} 63 else 64 {ast, issues} 65 end 66 end 67 68 defp traverse(ast, issues, _issue_meta) do 69 {ast, issues} 70 end 71 72 defp issues_for_with(clauses, line, issue_meta) do 73 issue_if_not_starting_with_pattern_clause(clauses, line, issue_meta) ++ 74 issue_if_not_ending_with_pattern_clause(clauses, line, issue_meta) 75 end 76 77 defp issue_if_not_starting_with_pattern_clause( 78 [{:<-, _meta, _args} | _rest], 79 _line, 80 _issue_meta 81 ) do 82 [] 83 end 84 85 defp issue_if_not_starting_with_pattern_clause(_clauses, line, issue_meta) do 86 [format_issue(issue_meta, message: @message_first_clause_not_pattern, line_no: line)] 87 end 88 89 defp issue_if_not_ending_with_pattern_clause(clauses, line, issue_meta) do 90 if length(clauses) > 1 and not match?({:<-, _, _}, Enum.at(clauses, -1)) do 91 [format_issue(issue_meta, message: @message_last_clause_not_pattern, line_no: line)] 92 else 93 [] 94 end 95 end 96 end