diff --git a/CHANGELOG.md b/CHANGELOG.md index ce56518d..528daf90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,16 +2,27 @@ **Note** Styler's only public API is its usage as a formatter plugin. While you're welcome to play with its internals, they can and will change without that change being reflected in Styler's semantic version. - ## main +## 1.1.1 + ### Improvements +* `unless`: rewrite `unless a |> b |> c` as `unless !(a |> b() |> c())` rather than `unless a |> b() |> c() |> Kernel.!()` (h/t @gregmefford) + +## 1.1.0 + +### 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 * `pipes`: optimizations reducing 2 pipes to 1 no longer squeeze all pipes onto one line (#180) +* `if`: fix infinite loop rewriting negated if with empty do body `if x != y, do: (), else: :ok` (#196, h/t @itamm15) ## 1.0.0 diff --git a/README.md b/README.md index 96cc3915..00c17489 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ Add `:styler` as a dependency to your project's `mix.exs`: ```elixir def deps do [ - {:styler, "~> 1.0.0-rc.1", only: [:dev, :test], runtime: false}, + {:styler, "~> 1.1", only: [:dev, :test], runtime: false}, ] end ``` diff --git a/docs/control_flow_macros.md b/docs/control_flow_macros.md index 39243034..70a85460 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 dbfe5ab8..ce3a849c 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -176,30 +176,27 @@ 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} # drop `else: nil` [head, [do_block, {_, {:__block__, _, [nil]}}]] -> @@ -264,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 @@ -327,5 +324,10 @@ 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({:in, m, [_, _]} = ast), do: {:not, m, [ast]} + defp invert({_, m, _} = ast), do: {:!, [line: m[:line]], [ast]} end diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index 41bed9f0..c88f8555 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 d2974650..20761b3a 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/mix.exs b/mix.exs index 6490d02b..7aa62597 100644 --- a/mix.exs +++ b/mix.exs @@ -12,7 +12,7 @@ defmodule Styler.MixProject do use Mix.Project # Don't forget to bump the README when doing non-patch version changes - @version "1.0.0" + @version "1.1.1" @url "https://github.com/adobe/elixir-styler" def project do diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index b2a4e348..10c6d937 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -822,24 +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 + describe "unless to if" do + test "inverts all the things" do + assert_style( + """ + unless !! not true do + a + else + b + end + """, + """ + if true do + a + else + b + end + """ + ) - test "Credo.Check.Refactor.UnlessWithElse" do for negator <- ["!", "not "] do assert_style( """ @@ -894,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") @@ -932,7 +931,73 @@ 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()), do: x" + end + + test "in" do + assert_style "unless a in b, do: x", "if a not in b, 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") @@ -976,44 +1041,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 f0eb84a0..02688247 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 61a11f2a..f87c7bd3 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)