elixir-tools / credo-language-server

LSP implementation for Credo.
MIT License
98 stars 11 forks source link

feat: code action for refactoring FilterFilter #19

Open wesleimp opened 1 year ago

wesleimp commented 1 year ago

Reference: https://hexdocs.pm/credo/Credo.Check.Refactor.FilterFilter.html

novaugust commented 1 year ago

the credo example docs make it look like it'll always be easy to combine a filter-filter, buuut there's a reason styler doesn't implement this rule...

# Not so easy to rewrite
stuff
|> Enum.filter(fn 
  x when is_list(x) -> good_list?(x)
  %{} = map -> keep_map?(map)
  _ -> true
end)
|> Enum.filter(fn  
  {:ok, good_stuff} -> true
  {:error, try_again} -> remote_check?(try_again)
  _ -> false
end)

without the knowledge of a programmer, the only way i've come up with for an ast rewrite to always solve this problem would be something semi hideous like:

stuff
|> Enum.filter(fn x ->
  a = fn 
    x when is_list(x) -> good_list?(x)
    %{} = map -> keep_map?(map)
    _ -> true
  end

  b = fn
    {:ok, good_stuff} -> true
    {:error, try_again} -> remote_check?(try_again)
    _ -> false
  end

  a.(x) && b.(x)
end)

at which point it's like, eugh. you lost so much in readability and gained whatever in list traversal speed. what would joe armstrong say?

that said, you could certainly come up with smarter and better rewrites for trivial situations like what credo presented. i'm just showing the worst-case fallback

so, that's why styler doesn't implement / care about this rule and ones like it

mhanberg commented 1 year ago

yeah, would probably be best to return a "sorry not sorry" response if the callback was complex like in your example, but combine them otherwise.

mhanberg commented 1 year ago

if you had the non-refactorable version, credo should be like "wut plz stop" 🤣