Remove unless from codebases

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

docs/control_flow_macros.md

@@ -11,21 +11,22 @@ 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...
 
 > `with` great power comes great responsibility
 >
 > - 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.

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

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

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"

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(
         """

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

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)