diff --git a/CHANGELOG.md b/CHANGELOG.md index 01483d6b..938ef830 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ **this is a big one!** please report any issues :) #135 * `if`/`unless`: invert if and unless with `!=` or `!==`, like we do for `!` and `not` #132 * `@derive`: move `@derive` before `defstruct|schema|embedded_schema` declarations (fixes compiler warning!) #134 +* strings: rewrite double-quoted strings to use `~s` when there's 4+ escaped double-quotes + (`"\"\"\"\""` -> `~s("""")`) (`Credo.Check.Readability.StringSigils`) #146 ### Fixes diff --git a/README.md b/README.md index 49ef2452..69dc6229 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,7 @@ Disabling the rules means updating your `.credo.exs` depending on your configura {Credo.Check.Readability.SinglePipe, false}, # **potentially breaks compilation** - see **Troubleshooting** section below {Credo.Check.Readability.StrictModuleLayout, false}, +{Credo.Check.Readability.StringSigils, false}, {Credo.Check.Readability.UnnecessaryAliasExpansion, false}, {Credo.Check.Readability.WithSingleClause, false}, {Credo.Check.Refactor.CaseTrivialMatches, false}, diff --git a/lib/style/single_node.ex b/lib/style/single_node.ex index a35bdee9..bc8fc692 100644 --- a/lib/style/single_node.ex +++ b/lib/style/single_node.ex @@ -18,6 +18,7 @@ defmodule Styler.Style.SingleNode do * Credo.Check.Readability.LargeNumbers * Credo.Check.Readability.ParenthesesOnZeroArityDefs * Credo.Check.Readability.PreferImplicitTry + * Credo.Check.Readability.StringSigils * Credo.Check.Readability.WithSingleClause * Credo.Check.Refactor.CaseTrivialMatches * Credo.Check.Refactor.CondStatements @@ -27,6 +28,8 @@ defmodule Styler.Style.SingleNode do @behaviour Styler.Style + @closing_delimiters [~s|"|, ")", "}", "|", "]", "'", ">", "/"] + alias Styler.Zipper def run({node, meta}, ctx), do: {:cont, {style(node), meta}, ctx} @@ -34,10 +37,46 @@ defmodule Styler.Style.SingleNode do # as of 1.15, elixir's formatter takes care of this for us. if Version.match?(System.version(), "< 1.15.0-dev") do # 'charlist' => ~c"charlist" - defp style({:__block__, meta, [chars]} = node) when is_list(chars) do - if meta[:delimiter] == "'", - do: {:sigil_c, Keyword.put(meta, :delimiter, "\""), [{:<<>>, [line: meta[:line]], [List.to_string(chars)]}, []]}, - else: node + defp style({:__block__, [{:delimiter, ~s|'|} | meta], [chars]}) when is_list(chars), + do: {:sigil_c, [{:delimiter, ~s|"|} | meta], [{:<<>>, [line: meta[:line]], [List.to_string(chars)]}, []]} + end + + # rewrite double-quote strings with >= 4 escaped double-quotes as sigils + defp style({:__block__, [{:delimiter, ~s|"|} | meta], [string]} = node) when is_binary(string) do + # running a regex against every double-quote delimited string literal in a codebase doesn't have too much impact + # on adobe's internal codebase, but perhaps other codebases have way more literals where this'd have an impact? + if string =~ ~r/".*".*".*"/ do + # choose whichever delimiter would require the least # of escapes, + # ties being broken by our stylish ordering of delimiters (reflected in the 1-8 values) + {closer, _} = + string + |> String.codepoints() + |> Stream.filter(&(&1 in @closing_delimiters)) + |> Stream.concat(@closing_delimiters) + |> Enum.frequencies() + |> Enum.min_by(fn + {~s|"|, count} -> {count, 1} + {")", count} -> {count, 2} + {"}", count} -> {count, 3} + {"|", count} -> {count, 4} + {"]", count} -> {count, 5} + {"'", count} -> {count, 6} + {">", count} -> {count, 7} + {"/", count} -> {count, 8} + end) + + delimiter = + case closer do + ")" -> "(" + "}" -> "{" + "]" -> "[" + ">" -> "<" + closer -> closer + end + + {:sigil_s, [{:delimiter, delimiter} | meta], [{:<<>>, [line: meta[:line]], [string]}, []]} + else + node end end diff --git a/test/style/single_node_test.exs b/test/style/single_node_test.exs index 97d4e0c6..cca3d92b 100644 --- a/test/style/single_node_test.exs +++ b/test/style/single_node_test.exs @@ -19,6 +19,22 @@ defmodule Styler.Style.SingleNodeTest do end end + test "string sigil rewrites" do + assert_style ~s|""| + assert_style ~s|"\\""| + assert_style ~s|"\\"\\""| + assert_style ~s|"\\"\\"\\""| + assert_style ~s|"\\"\\"\\"\\""|, ~s|~s("""")| + # choose closing delimiter wisely, based on what has the least conflicts, in the styliest order + assert_style ~s/"\\"\\"\\"\\" )"/, ~s/~s{"""" )}/ + assert_style ~s/"\\"\\"\\"\\" })"/, ~s/~s|"""" })|/ + assert_style ~s/"\\"\\"\\"\\" |})"/, ~s/~s["""" |})]/ + assert_style ~s/"\\"\\"\\"\\" ]|})"/, ~s/~s'"""" ]|})'/ + assert_style ~s/"\\"\\"\\"\\" ']|})"/, ~s/~s<"""" ']|})>/ + assert_style ~s/"\\"\\"\\"\\" >']|})"/, ~s|~s/"""" >']\|})/| + assert_style ~s/"\\"\\"\\"\\" \/>']|})"/, ~s|~s("""" />']\|}\\))| + end + test "{Map/Keyword}.merge with a single static key" do for module <- ~w(Map Keyword) do assert_style("#{module}.merge(foo, %{one_key: :bar})", "#{module}.put(foo, :one_key, :bar)")