Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rewrite double quote strings with many escapes to use ~s #146

Merged
merged 1 commit into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
47 changes: 43 additions & 4 deletions lib/style/single_node.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -27,17 +28,55 @@ 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}

# 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

Expand Down
16 changes: 16 additions & 0 deletions test/style/single_node_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -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)")
Expand Down
Loading