diff --git a/CHANGELOG.md b/CHANGELOG.md index ce20b8d..616ca03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ they can and will change without that change being reflected in Styler's semanti ### Improvements +The big change here is the rewrite/removal of `unless` due to [unless "eventually" being deprecated](https://github.com/elixir-lang/elixir/pull/13769#issuecomment-2334878315). Thanks to @janpieper and @ypconstante for bringing this up in #190. + +* `unless`: rewrite all `unless` to `if` (#190) * `pipes`: optimize `|> Stream.{each|map}(fun) |> Stream.run()` to `|> Enum.each(fun)` ### Fixes diff --git a/docs/control_flow_macros.md b/docs/control_flow_macros.md index 3924303..70a8546 100644 --- a/docs/control_flow_macros.md +++ b/docs/control_flow_macros.md @@ -11,13 +11,15 @@ The number of "blocks" in Elixir means there are many ways to write semantically We believe readability is enhanced by using the simplest api possible, whether we're talking about internal module function calls or standard-library macros. -### use `case`, `if`, or `unless` when... +### use `case`, `if`, or `cond` when... We advocate for `case` and `if` as the first tools to be considered for any control flow as they are the two simplest blocks. If a branch _can_ be expressed with an `if` statement, it _should_ be. Otherwise, `case` is the next best choice. In situations where developers might reach for an `if/elseif/else` block in other languages, `cond do` should be used. (`cond do` seems to see a paucity of use in the language, but many complex nested expressions or with statements can be improved by replacing them with a `cond do`). -`unless` is a special case of `if` meant to make code read as natural-language (citation needed). While it sometimes succeeds in this goal, its absence in most programming languages often makes it feel cumbersome to programmers with non-Ruby backgrounds. Thankfully, with Styler's help developers don't need to ever reach for `unless` - expressions that are "simpler" with its use are automatically rewritten to use it. +### use `unless` when... + +Never! `unless` [is being deprecated](https://github.com/elixir-lang/elixir/pull/13769#issuecomment-2334878315) and so should not be used. ### use `with` when... @@ -25,7 +27,6 @@ We advocate for `case` and `if` as the first tools to be considered for any cont > > - Uncle Ben - As the most powerful of the Kernel control-flow expressions, `with` requires the most cognitive overhead to understand. Its power means that we can use it as a replacement for anything we might express using a `case`, `if`, or `cond` (especially with the liberal application of small private helper functions). Unfortunately, this has lead to a proliferation of `with` in codebases where simpler expressions would have sufficed, meaning a lot of Elixir code ends up being harder for readers to understand than it needs to be. diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index ffb0ba0..af2d6a8 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -176,31 +176,24 @@ defmodule Styler.Style.Blocks do end end - def run({{:unless, m, children}, _} = zipper, ctx) do - case children do - # Credo.Check.Refactor.UnlessWithElse - [{_, hm, _} = head, [_, _] = do_else] -> - zipper |> Zipper.replace({:if, m, [{:!, hm, [head]}, do_else]}) |> run(ctx) - - # Credo.Check.Refactor.NegatedConditionsInUnless - [negator, [{do_, do_body}]] when is_negator(negator) -> - zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, do_body}]]}) |> run(ctx) - - _ -> - {:cont, zipper, ctx} - end + def run({{:unless, m, [head, do_else]}, _} = zipper, ctx) do + zipper + |> Zipper.replace({:if, m, [invert(head), do_else]}) + |> run(ctx) end def run({{:if, m, children}, _} = zipper, ctx) do case children do + # double negator + # if !!x, do: y[, else: ...] => if x, do: y[, else: ...] + [{_, _, [nb]} = na, do_else] when is_negator(na) and is_negator(nb) -> + zipper |> Zipper.replace({:if, m, [invert(nb), do_else]}) |> run(ctx) + # Credo.Check.Refactor.NegatedConditionsWithElse + # if !x, do: y, else: z => if x, do: z, else: y [negator, [{do_, do_body}, {else_, else_body}]] when is_negator(negator) -> zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, else_body}, {else_, do_body}]]}) |> run(ctx) - # if not x, do: y => unless x, do: y - [negator, [do_block]] when is_negator(negator) -> - zipper |> Zipper.replace({:unless, m, [invert(negator), [do_block]]}) |> run(ctx) - # drop `else end` [head, [do_block, {_, {:__block__, _, []}}]] -> {:cont, Zipper.replace(zipper, {:if, m, [head, [do_block]]}), ctx} @@ -268,7 +261,7 @@ defmodule Styler.Style.Blocks do # c # d # ) - # @TODO would be nice to changeto + # @TODO would be nice to change to # a # b # c @@ -331,5 +324,16 @@ defmodule Styler.Style.Blocks do defp invert({:!=, m, [a, b]}), do: {:==, m, [a, b]} defp invert({:!==, m, [a, b]}), do: {:===, m, [a, b]} - defp invert({_, _, [expr]}), do: expr + defp invert({:==, m, [a, b]}), do: {:!=, m, [a, b]} + defp invert({:===, m, [a, b]}), do: {:!==, m, [a, b]} + defp invert({:!, _, [condition]}), do: condition + defp invert({:not, _, [condition]}), do: condition + + defp invert({fun, m, _} = ast) do + meta = [line: m[:line]] + + if fun == :|>, + do: {:|>, meta, [ast, {{:., meta, [Kernel, :!]}, meta, []}]}, + else: {:!, meta, [ast]} + end end diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index e385eb9..623ce5f 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -135,7 +135,7 @@ defmodule Styler.Style.ModuleDirectives do defp moduledoc({:__aliases__, m, aliases}) do name = aliases |> List.last() |> to_string() # module names ending with these suffixes will not have a default moduledoc appended - unless String.ends_with?(name, ~w(Test Mixfile MixProject Controller Endpoint Repo Router Socket View HTML JSON)) do + if !String.ends_with?(name, ~w(Test Mixfile MixProject Controller Endpoint Repo Router Socket View HTML JSON)) do Style.set_line(@moduledoc_false, m[:line] + 1) end end diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index d297465..20761b3 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -128,6 +128,8 @@ defmodule Styler.Style.Pipes do # |> ... var_name = case fun do + # unless will be rewritten to `if` statements in the Blocks Style + :unless -> :if fun when is_atom(fun) -> fun {:., _, [{:__aliases__, _, _}, fun]} when is_atom(fun) -> fun _ -> "block" diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 400fd99..14fd9e8 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -822,42 +822,25 @@ defmodule Styler.Style.BlocksTest do end end - describe "if/unless" do - test "drops if else nil" do - assert_style("if a, do: b, else: nil", "if a, do: b") - - assert_style("if a do b else nil end", """ - if a do - b - end - """) - end - - test "if not => unless" do - assert_style("if not x, do: y", "unless x, do: y") - assert_style("if !x, do: y", "unless x, do: y") - assert_style("if !!x, do: y", "if x, do: y") - end - - test "regression: negation with empty do body" do + describe "unless to if" do + test "inverts all the things" do assert_style( """ - if a != b do - # comment + unless !! not true do + a else - :ok + b end """, """ - if a == b do - # comment - :ok + if true do + a + else + b end """ ) - end - test "Credo.Check.Refactor.UnlessWithElse" do for negator <- ["!", "not "] do assert_style( """ @@ -912,9 +895,7 @@ defmodule Styler.Style.BlocksTest do end """ ) - end - test "Credo.Check.Refactor.NegatedConditionsInUnless" do for negator <- ["!", "not "] do assert_style("unless #{negator} foo, do: :bar", "if foo, do: :bar") @@ -950,7 +931,69 @@ defmodule Styler.Style.BlocksTest do end end - test "Credo.Check.Refactor.NegatedConditionsWithElse" do + test "unless with pipes" do + assert_style "unless a |> b() |> c(), do: x", "if a |> b() |> c() |> Kernel.!(), do: x" + end + end + + describe "if" do + test "drops else nil" do + assert_style("if a, do: b, else: nil", "if a, do: b") + + assert_style("if a do b else nil end", """ + if a do + b + end + """) + + assert_style( + """ + if a != b do + # comment + else + :ok + end + """, + """ + if a == b do + # comment + :ok + end + """ + ) + end + + test "double negator rewrites" do + for a <- ~w(not !), block <- ["do: z", "do: z, else: zz"] do + assert_style "if #{a} (x != y), #{block}", "if x == y, #{block}" + assert_style "if #{a} (x !== y), #{block}", "if x === y, #{block}" + assert_style "if #{a} ! x, #{block}", "if x, #{block}" + assert_style "if #{a} not x, #{block}", "if x, #{block}" + end + + assert_style("if not x, do: y", "if not x, do: y") + assert_style("if !x, do: y", "if !x, do: y") + + assert_style( + """ + if !!val do + a + else + b + end + """, + """ + if val do + a + else + b + end + """ + ) + end + + test "single negator do/else swaps" do + # covers Credo.Check.Refactor.NegatedConditionsWithElse for negator <- ["!", "not "] do assert_style("if #{negator}foo, do: :bar, else: :baz", "if foo, do: :baz, else: :bar") @@ -994,44 +1037,6 @@ defmodule Styler.Style.BlocksTest do end end - test "recurses" do - assert_style( - """ - if !!val do - a - else - b - end - """, - """ - if val do - a - else - b - end - """ - ) - - assert_style( - """ - unless !! not true do - a - else - b - end - """, - """ - if true do - a - else - b - end - """ - ) - - assert_style("if not (a != b), do: c", "if a == b, do: c") - end - test "comments and flips" do assert_style( """ diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 5bd24c9..49763ea 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -228,12 +228,12 @@ defmodule Styler.Style.PipesTest do |> wee() """, """ - unless_result = - unless foo do + if_result = + if !foo do bar end - wee(unless_result) + wee(if_result) """ ) end diff --git a/test/support/style_case.ex b/test/support/style_case.ex index 61a11f2..f87c7bd 100644 --- a/test/support/style_case.ex +++ b/test/support/style_case.ex @@ -70,7 +70,7 @@ defmodule Styler.StyleCase do # body blocks - for example, the block node for an anonymous function - don't have line meta # yes, i just did `&& case`. sometimes it's funny to write ugly things in my project that's all about style. # i believe they calls that one "irony" - is_body_block? = + body_block? = node == :__block__ && case up && Zipper.node(up) do # top of a snippet @@ -99,7 +99,7 @@ defmodule Styler.StyleCase do end end - unless line || is_body_block? do + if is_nil(line) and not body_block? do IO.puts("missing `:line` meta in node:") dbg(ast)