diff --git a/lib/style.ex b/lib/style.ex index 63e5d81..4d9264c 100644 --- a/lib/style.ex +++ b/lib/style.ex @@ -170,6 +170,9 @@ defmodule Styler.Style do {directive, updated_meta, children} end + def first_line({:__block__, m, [{_, cm, _} | _]}), do: m[:line] || cm[:line] + def first_line(node), do: meta(node)[:line] + def max_line([_ | _] = list), do: list |> List.last() |> max_line() def max_line(ast) do @@ -198,7 +201,7 @@ defmodule Styler.Style do {nodes, shifted_comments, comments, _line} = Enum.reduce(nodes, {[], [], comments, first_line}, fn node, {n_acc, c_acc, comments, move_to_line} -> meta = meta(node) - line = meta[:line] + line = first_line(node) last_line = max_line(node) {mine, comments} = comments_for_lines(comments, line, last_line) @@ -223,7 +226,7 @@ defmodule Styler.Style do @doc """ Returns all comments "for" a node, including on the line before it. see `comments_for_lines` for more """ - def comments_for_node({_, m, _} = node, comments), do: comments_for_lines(comments, m[:line], max_line(node)) + def comments_for_node(node, comments), do: comments_for_lines(comments, first_line(node), max_line(node)) @doc """ Gets all comments in range start_line..last_line, and any comments immediately before start_line.s diff --git a/lib/style/blocks.ex b/lib/style/blocks.ex index 216930b..6331745 100644 --- a/lib/style/blocks.ex +++ b/lib/style/blocks.ex @@ -34,11 +34,13 @@ defmodule Styler.Style.Blocks do # case statement with exactly 2 `->` cases # rewrite to `if` if it's any of 3 trivial cases - def run({{:case, _, [head, [{_, [{:->, _, [[lhs_a], a]}, {:->, _, [[lhs_b], b]}]}]]}, _} = zipper, ctx) do + def run({{:case, m, [head, [{_, [{:->, _, [[lhs_a], _]} = a, {:->, _, [[lhs_b], _]} = b]}]]}, _} = zipper, ctx) do + end_line = m[:end][:line] + case {lhs_a, lhs_b} do - {{_, _, [true]}, {_, _, [false]}} -> if_ast(zipper, head, a, b, ctx) - {{_, _, [true]}, {:_, _, _}} -> if_ast(zipper, head, a, b, ctx) - {{_, _, [false]}, {_, _, [true]}} -> if_ast(zipper, head, b, a, ctx) + {{_, _, [true]}, {_, _, [false]}} -> arrows_to_if(zipper, head, a, b, end_line, ctx) + {{_, _, [true]}, {:_, _, _}} -> arrows_to_if(zipper, head, a, b, end_line, ctx) + {{_, _, [false]}, {_, _, [true]}} -> arrows_to_if(zipper, head, b, a, end_line, ctx) _ -> {:cont, zipper, ctx} end end @@ -78,7 +80,7 @@ defmodule Styler.Style.Blocks do {:cont, zipper, ctx} end - def run({{:cond, _, [[{do_, clauses}]]}, _} = zipper, ctx) do + def run({{:cond, m, [[{do_, clauses}]]}, _} = zipper, ctx) do # ensure all final `atom -> final_clause` use `true` for consistency. # `:else` is cute but consistency is all. rewrite_literal_to_true = fn @@ -95,9 +97,12 @@ defmodule Styler.Style.Blocks do end case List.update_at(clauses, -1, rewrite_literal_to_true) do - # # Credo.Check.Refactor.CondStatements - [{:->, _, [[head], a]}, {:->, _, [[{:__block__, _, [true]}], b]}] -> if_ast(zipper, head, a, b, ctx) - clauses -> {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} + # Credo.Check.Refactor.CondStatements + [{:->, _, [[head], _]} = a, {:->, _, [[{:__block__, _, [true]}], _]} = b] -> + arrows_to_if(zipper, head, a, b, m[:end][:line], ctx) + + clauses -> + {:cont, Zipper.replace_children(zipper, [[{do_, clauses}]]), ctx} end end @@ -190,6 +195,10 @@ defmodule Styler.Style.Blocks do # 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) -> + # end of expression hack ensure that the else body keeps dangling comments its block + else_line = Style.meta(else_)[:line] + do_body = Macro.update_meta(do_body, &Keyword.put(&1, :end_of_expression, line: else_line, newlines: 1)) + zipper |> Zipper.replace({:if, m, [invert(negator), [{do_, else_body}, {else_, do_body}]]}) |> run(ctx) # drop `else end` @@ -202,9 +211,7 @@ defmodule Styler.Style.Blocks do [head, [do_, else_]] -> if Style.max_line(do_) > Style.max_line(else_) do - # we inverted the if/else blocks of this `if` statement in a previous pass (due to negators or unless) - # shift comments etc to make it happy now - if_ast(zipper, head, do_, else_, ctx) + organize_if(zipper, head, do_, else_, ctx) else {:cont, zipper, ctx} end @@ -366,50 +373,59 @@ defmodule Styler.Style.Blocks do defp nodes_equivalent?(a, b), do: Style.without_meta(a) == Style.without_meta(b) - defp if_ast(zipper, head, {_, _, _} = do_body, {_, _, _} = else_body, ctx) do - do_ = {{:__block__, [line: nil], [:do]}, do_body} - else_ = {{:__block__, [line: nil], [:else]}, else_body} - if_ast(zipper, head, do_, else_, ctx) + # hacks comments above the arrows to have the same line number as the start of the body, + # and hacks the body of the last of a/b to have an end of expression equal to where the `end` keyword is to make sure + # dangling comments get caught + # would be lovely to not hack things so hard but c'est la vie for now + defp arrows_to_if(zipper, head, {:->, am, [_, a]}, {:->, bm, [_, b]}, end_line, ctx) do + ctx = + ctx + |> Map.update!(:comments, &lower_arrow_comments_to_body(&1, am, a)) + |> Map.update!(:comments, &lower_arrow_comments_to_body(&1, bm, b)) + + # hacking the end_of_expression helps ensure that the (previously) last clause catches dangling comments + [a, b] = + if Style.first_line(a) < Style.first_line(b) do + b = Macro.update_meta(b, &Keyword.put(&1, :end_of_expression, line: end_line, newlines: 1)) + [a, b] + else + a = Macro.update_meta(a, &Keyword.put(&1, :end_of_expression, line: end_line, newlines: 1)) + [a, b] + end + + do_ = {{:__block__, [line: nil], [:do]}, a} + else_ = {{:__block__, [line: nil], [:else]}, b} + organize_if(zipper, head, do_, else_, ctx) end - defp if_ast(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do - line = meta[:line] - # ... why am i doing this again? hmm. + defp lower_arrow_comments_to_body(comments, arrow_meta, body) do + arrow_line = arrow_meta[:line] + + if Style.first_line(body) == arrow_line do + comments + else + {mine, rest} = Style.comments_for_lines(comments, arrow_line, arrow_line) + mine = Enum.map(mine, &%{&1 | line: &1.line + 1}) + Enum.sort_by(rest ++ mine, & &1.line) + end + end + + defp organize_if(zipper, {_, meta, _} = head, {do_kw, do_body}, {else_kw, else_body}, ctx) do + head_line = meta[:line] + + {[do_body, else_body], comments} = Style.order_line_meta_and_comments([do_body, else_body], ctx.comments, head_line) + + else_line = Style.max_line(do_body) + end_line = Style.max_line(else_body) + 1 + # clean up the dangling comments hack if this was a conversion do_body = Macro.update_meta(do_body, &Keyword.delete(&1, :end_of_expression)) else_body = Macro.update_meta(else_body, &Keyword.delete(&1, :end_of_expression)) - max_do_line = Style.max_line(do_body) - max_else_line = Style.max_line(else_body) - end_line = max(max_do_line, max_else_line) - - # Change ast meta and comment lines to fit the `if` ast - {do_, else_, comments} = - if max_do_line >= max_else_line do - # we're swapping the ordering of two blocks of code - # and so must swap the lines of the ast & comments to keep comments where they belong! - # the math is: move B up by the length of A, and move A down by the length of B plus one (for the else keyword) - else_size = max_else_line - line - do_size = max_do_line - max_else_line - - shifts = [ - # move comments in the `else_body` down by the size of the `do_body` - {line..max_else_line, do_size}, - # move comments in `do_body` up by the size of the `else_body` - {(max_else_line + 1)..max_do_line, -else_size} - ] - - do_ = {Style.set_line(do_kw, line), Style.shift_line(do_body, -else_size)} - else_ = {Style.set_line(else_kw, max_else_line), Style.shift_line(else_body, do_size)} - {do_, else_, Style.shift_comments(ctx.comments, shifts)} - else - # much simpler case -- just scootch things in the else down by 1 for the `else` keyword. - do_ = {{:__block__, [line: line], [:do]}, do_body} - else_ = Style.shift_line({{:__block__, [line: max_do_line], [:else]}, else_body}, 1) - {do_, else_, Style.shift_comments(ctx.comments, max_do_line..max_else_line, 1)} - end + do_ = {Style.set_line(do_kw, head_line), do_body} + else_ = {Style.set_line(else_kw, else_line), else_body} zipper - |> Zipper.replace({:if, [do: [line: line], end: [line: end_line], line: line], [head, [do_, else_]]}) + |> Zipper.replace({:if, [do: [line: head_line], end: [line: end_line], line: head_line], [head, [do_, else_]]}) |> run(%{ctx | comments: comments}) end diff --git a/lib/style/configs.ex b/lib/style/configs.ex index d83b447..3583c85 100644 --- a/lib/style/configs.ex +++ b/lib/style/configs.ex @@ -50,16 +50,6 @@ defmodule Styler.Style.Configs do def run({{:config, cfm, [_, _ | _]} = config, zm}, %{mix_config?: true, comments: comments} = ctx) do # all of these list are reversed due to the reduce {configs, assignments, rest} = accumulate(zm.r, [], []) - # @TODO - # okay so comments between nodes that we moved....... - # lets just push them out of the way (???). so - # 1. figure out first/last possible lines we're talking about here - # 2. only pass comments in that range off - # 3. split those comments into "moved, didn't move" - # 4. for any "didn't move" comments... move them to the top? - # - # also, should i just do a scan of the configs ++ assignments, and see if any of them have lines out of order, - # and decide from there whether or not i want to do set_lines configs = [config | configs] @@ -80,16 +70,26 @@ defmodule Styler.Style.Configs do |> Style.reset_newlines() |> Enum.concat(configs) - {nodes, comments} = + {nodes, comments, rest} = if changed?(nodes) do - # after running, this block should take up the same # of lines that it did before - # the first node of `rest` is greater than the highest line in configs, assignments # config line is the first line to be used as part of this block {node_comments, _} = Style.comments_for_node(config, comments) first_line = min(List.first(node_comments)[:line] || cfm[:line], cfm[:line]) - Style.order_line_meta_and_comments(nodes, comments, first_line) + + # Sorting and re-spacing can make the block taller (config groups gain blank lines between them). + # reodering means the block can grow past `rest` and its comments, causing the comments for `rest` to get sucked up into our block. + max_before_ordering = [config | configs ++ assignments] |> Enum.map(&Style.max_line/1) |> Enum.max() + {block_comments, tail_comments} = Enum.split_while(comments, &(&1.line <= max_before_ordering)) + + {nodes, block_comments} = Style.order_line_meta_and_comments(nodes, block_comments, first_line) + + delta = Style.max_line(nodes) - max_before_ordering + tail_comments = Enum.map(tail_comments, &%{&1 | line: &1.line + delta}) + rest = Style.shift_line(rest, delta) + + {nodes, Enum.sort_by(block_comments ++ tail_comments, & &1.line), rest} else - {nodes, comments} + {nodes, comments, rest} end [config | left_siblings] = Enum.reverse(nodes, zm.l) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index fac6189..ad44b86 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -121,7 +121,7 @@ defmodule Styler.Style.ModuleDirectives do # we want only-child literal block to be handled in the only-child catch-all. it means someone did a weird # (that would be a literal, so best case someone wrote a string and forgot to put `@moduledoc` before it) {:__block__, _, [_, _ | _]} -> - {:skip, organize_directives(body_zipper, moduledoc), ctx} + organize_directives(body_zipper, ctx, moduledoc) # a module whose only child is a moduledoc. nothing to do here! # seems weird at first blush but lots of projects/libraries do this with their root namespace module @@ -131,12 +131,9 @@ defmodule Styler.Style.ModuleDirectives do # There's only one child, and it's not a moduledoc. Conditionally add a moduledoc, then style the only_child only_child -> if moduledoc do - zipper = - body_zipper - |> Zipper.replace({:__block__, [], [moduledoc, only_child]}) - |> organize_directives() - - {:skip, zipper, ctx} + body_zipper + |> Zipper.replace({:__block__, [], [moduledoc, only_child]}) + |> organize_directives(ctx) else do_run(body_zipper, ctx) end @@ -148,7 +145,7 @@ defmodule Styler.Style.ModuleDirectives do defp do_run({{directive, _, children}, _} = zipper, ctx) when directive in @directives and is_list(children) do # Need to be careful that we aren't getting false positives on variables or fns like `def import(foo)` or `alias = 1` case Style.ensure_block_parent(zipper) do - {:ok, zipper} -> {:skip, zipper |> Zipper.up() |> organize_directives(), ctx} + {:ok, zipper} -> zipper |> Zipper.up() |> organize_directives(ctx) # not actually a directive! carry on. :error -> {:cont, zipper, ctx} end @@ -229,10 +226,12 @@ defmodule Styler.Style.ModuleDirectives do end end - defp organize_directives(parent, moduledoc \\ nil) do + defp organize_directives(parent, ctx, moduledoc \\ nil) do + original_children = Zipper.children(parent) + comments = ctx.comments + acc = - parent - |> Zipper.children() + original_children |> Enum.reduce(@env, fn {:@, _, [{attr_directive, _, _}]} = ast, acc when attr_directive in @attr_directives -> # attr_directives are moved above aliases, so we need to expand them @@ -281,19 +280,35 @@ defmodule Styler.Style.ModuleDirectives do acc.require ] |> Stream.concat() - |> fix_line_numbers(List.first(nondirectives)) + |> Enum.to_list() + + nodes = directives ++ nondirectives + + # have to compare without meta due to newlines being reset while grouping directives + {{directives, nondirectives}, comments} = + if Enum.empty?(nodes) or Style.without_meta(nodes) == Style.without_meta(original_children) do + {{directives, nondirectives}, comments} + else + first_line = nodes |> Enum.map(&Style.meta(&1)[:line]) |> Enum.min() + {nodes, comments} = Style.order_line_meta_and_comments(nodes, comments, first_line) + # This is shameful. I'm sorry + {Enum.split(nodes, length(directives)), comments} + end # the # of aliases can be decreased during sorting - if there were any, we need to be sure to write the deletion - if Enum.empty?(directives) do - Zipper.replace_children(parent, nondirectives) - else - # this ensures we continue the traversal _after_ any directives - parent - |> Zipper.replace_children(directives) - |> Zipper.down() - |> Zipper.rightmost() - |> Zipper.insert_siblings(nondirectives) - end + zipper = + if Enum.empty?(directives) do + Zipper.replace_children(parent, nondirectives) + else + # this ensures we continue the traversal _after_ any directives + parent + |> Zipper.replace_children(directives) + |> Zipper.down() + |> Zipper.rightmost() + |> Zipper.insert_siblings(nondirectives) + end + + {:skip, zipper, %{ctx | comments: comments}} end # alias_env have to be recomputed after we've sorted our `alias` nodes @@ -548,65 +563,4 @@ defmodule Styler.Style.ModuleDirectives do |> Enum.map(&elem(&1, 0)) |> Style.reset_newlines() end - - # "Fixes" the line numbers of nodes who have had their orders changed via sorting or other methods. - # This "fix" simply ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly. - # - # The fix is rather naive, and simply enforces the following property on the code: - # A given node must have a line number less than the following node. - # Et voila! Comments behave much better. - # - # ## In Detail - # - # For example, given document - # - # 1: defmodule ... - # 2: alias B - # 3: # this is foo - # 4: def foo ... - # 5: alias A - # - # Sorting aliases the ast node for would put `alias A` (line 5) before `alias B` (line 2). - # - # 1: defmodule ... - # 5: alias A - # 2: alias B - # 3: # this is foo - # 4: def foo ... - # - # Elixir's document algebra would then encounter `line: 5` and immediately dump all comments with `line <= 5`, - # meaning after running through the formatter we'd end up with - # - # 1: defmodule - # 2: # hi - # 3: # this is foo - # 4: alias A - # 5: alias B - # 6: - # 7: def foo ... - # - # This function fixes that by seeing that `alias A` has a higher line number than its following sibling `alias B` and so - # updates `alias A`'s line to be preceding `alias B`'s line. - # - # Running the results of this function through the formatter now no longer dumps the comments prematurely - # - # 1: defmodule ... - # 2: alias A - # 3: alias B - # 4: # this is foo - # 5: def foo ... - defp fix_line_numbers(nodes, nil), do: fix_line_numbers(nodes, 999_999) - defp fix_line_numbers(nodes, {_, meta, _}), do: fix_line_numbers(nodes, meta[:line]) - defp fix_line_numbers(nodes, max), do: nodes |> Enum.reverse() |> do_fix_lines(max, []) - - defp do_fix_lines([], _, acc), do: acc - - defp do_fix_lines([{_, meta, _} = node | nodes], max, acc) do - line = meta[:line] - - # the -2 is just an ugly hack to leave room for one-liner comments and not hijack them. - if line > max, - do: do_fix_lines(nodes, max, [Style.shift_line(node, max - line - 2) | acc]), - else: do_fix_lines(nodes, line, [node | acc]) - end end diff --git a/lib/style/pipes.ex b/lib/style/pipes.ex index 9aceca1..ad908d2 100644 --- a/lib/style/pipes.ex +++ b/lib/style/pipes.ex @@ -110,17 +110,18 @@ defmodule Styler.Style.Pipes do # 3 |> rhs(...args) # => # 1 var = rhs(lhs, ...args) + comments = Style.displace_comments(ctx.comments, vm[:line]..Style.max_line(rhs)) oneline_assignment = Style.set_line({:=, am, [var, {fun, rhs_meta, [lhs | args]}]}, vm[:line]) - # skip so we don't re-traverse - {:cont, Zipper.replace(assignment_parent, oneline_assignment), ctx} + {:cont, Zipper.replace(assignment_parent, oneline_assignment), %{ctx | comments: comments}} _ -> # lhs # |> rhs(...args) # => # rhs(lhs, ...) + comments = Style.displace_comments(ctx.comments, lhs_line..Style.max_line(rhs)) oneline_function_call = Style.set_line({fun, rhs_meta, [lhs | args]}, lhs_line) - {:cont, Zipper.replace(single_pipe_zipper, oneline_function_call), ctx} + {:cont, Zipper.replace(single_pipe_zipper, oneline_function_call), %{ctx | comments: comments}} end end end diff --git a/test/style/blocks_test.exs b/test/style/blocks_test.exs index 1c0240a..bf8b8c9 100644 --- a/test/style/blocks_test.exs +++ b/test/style/blocks_test.exs @@ -208,9 +208,8 @@ defmodule Styler.Style.BlocksTest do if foo do # a :ok + # b end - - # b """ ) @@ -1260,13 +1259,101 @@ defmodule Styler.Style.BlocksTest do ) end end + end + + describe "block swap comment coverage" do + test "unless with multi-line do/else bodies" do + assert_style( + """ + unless a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + # b2 + b2 + end + """ + ) + end + + test "non-! negator (!=) with multi-line do/else bodies" do + assert_style( + """ + if a != b do + # d1 + d1 + # d2 + d2 + else + # e + e + end + """, + """ + if a == b do + # e + e + else + # d1 + d1 + # d2 + d2 + end + """ + ) + end - test "comments and flips" do + test "3-statement do body swaps below a 1-statement else" do + assert_style( + """ + if !a do + # x1 + x1 + # x2 + x2 + # x3 + x3 + else + # y + y + end + """, + """ + if a do + # y + y + else + # x1 + x1 + # x2 + x2 + # x3 + x3 + end + """ + ) + end + + test "dangling comment at the end of a swapped do body" do assert_style( """ if !a do # b b + # dangling else # c c @@ -1279,6 +1366,375 @@ defmodule Styler.Style.BlocksTest do else # b b + # dangling + end + """ + ) + end + + test "blank line between statements survives the swap" do + assert_style( + """ + if !a do + # b1 + b1 + + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + + # b2 + b2 + end + """ + ) + end + + test "leading comment on the if itself is untouched by the swap" do + assert_style( + """ + # leading comment + if !a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + # leading comment + if a do + # c + c + else + # b1 + b1 + # b2 + b2 + end + """ + ) + end + + test "double negator collapse leaves multi-line do/else bodies untouched" do + assert_style( + """ + if !!a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # b1 + b1 + # b2 + b2 + else + # c + c + end + """ + ) + end + + test "case to if with multi-line do/else bodies (false first)" do + assert_style( + """ + case foo do + false -> + # d1 + d1 + # d2 + d2 + true -> + # e + e + end + """, + """ + if foo do + # e + e + else + # d1 + d1 + # d2 + d2 + end + """ + ) + end + + test "cond to if with multi-line do body and 1-statement else" do + assert_style( + """ + cond do + a -> + # f1 + f1 + # f2 + f2 + true -> + # g + g + end + """, + """ + if a do + # f1 + f1 + # f2 + f2 + else + # g + g + end + """ + ) + end + + test "multi-line leading comment block on a clause header" do + assert_style( + """ + case foo do + # line 1 + # line 2 + false -> :error + true -> :ok + end + """, + """ + if foo do + :ok + else + # line 1 + # line 2 + :error + end + """ + ) + end + + test "wildcard else clause with multi-line do body and a leading comment" do + assert_style( + """ + case foo do + # a + true -> + b1 + b2 + _ -> + # c + c + end + """, + """ + if foo do + # a + b1 + b2 + else + # c + c + end + """ + ) + end + + test "with rewritten through case to if, with comments" do + assert_style( + """ + with true <- foo() do + # a + bar() + else + false -> + # b + baz() + end + """, + """ + if foo() do + # a + bar() + else + # b + baz() + end + """ + ) + end + + test "comment directly before the case, no blank line before the first clause" do + assert_style( + """ + # leading + case foo do + false -> :error + true -> :ok + end + """, + """ + # leading + if foo do + :ok + else + :error + end + """ + ) + end + + test "nested if inside a swapped multi-statement body" do + assert_style( + """ + if !a do + # b1 + b1 + + if x do + # nested + y + end + + # b2 + b2 + else + # c + c + end + """, + """ + if a do + # c + c + else + # b1 + b1 + + if x do + # nested + y + end + + # b2 + b2 + end + """ + ) + end + + test "asymmetric case with leading comments on both clauses and a trailing dangling comment" do + assert_style( + """ + case foo do + # a + false -> + # foo + d1 + d2 + # b + true -> + # bar + e + # dangling + end + """, + """ + if foo do + # b + # bar + e + # dangling + else + # a + # foo + d1 + d2 + end + """ + ) + end + + test "cond do rewrite with dangler" do + assert_style( + """ + cond do + # a + foo? -> + # foo + d1 + d2 + # b + true -> + # bar + e + # dangling + end + """, + """ + if foo? do + # a + # foo + d1 + d2 + else + # b + # bar + e + # dangling + end + """ + ) + end + + test "yet another" do + assert_style( + """ + case foo do + # a + true -> + # foo + d1 + # b + false -> + # bar + e + f + # dangling + end + """, + """ + if foo do + # a + # foo + d1 + else + # b + # bar + e + f + # dangling end """ ) diff --git a/test/style/configs_test.exs b/test/style/configs_test.exs index 05704ac..1f4340b 100644 --- a/test/style/configs_test.exs +++ b/test/style/configs_test.exs @@ -352,7 +352,7 @@ defmodule Styler.Style.ConfigsTest do ) end - test "big block regression #230" do + test "big block regression" do # The nodes are in reverse order assert_style( """ @@ -390,7 +390,7 @@ defmodule Styler.Style.ConfigsTest do ) end - test "phx config" do + test "phx.new config.exs" do assert_style( """ import Config @@ -430,4 +430,148 @@ defmodule Styler.Style.ConfigsTest do ) end end + + test "phx.new config.exs" do + assert_style( + """ + # This file is responsible for configuring your application + # and its dependencies with the aid of the Config module. + # + # This configuration file is loaded before any dependency and + # is restricted to this project. + + # General application configuration + import Config + + config :phx_new, + ecto_repos: [PhxNew.Repo], + generators: [timestamp_type: :utc_datetime] + + # Configures the endpoint + config :phx_new, PhxNewWeb.Endpoint, + url: [host: "localhost"], + adapter: Bandit.PhoenixAdapter, + pubsub_server: PhxNew.PubSub, + live_view: [signing_salt: "QdK6mkm/"] + + # Configures the mailer + # + # By default it uses the "Local" adapter which stores the emails + # locally. You can see the emails in your browser, at "/dev/mailbox". + # + # For production it's recommended to configure a different adapter + # at the `config/runtime.exs`. + config :phx_new, PhxNew.Mailer, adapter: Swoosh.Adapters.Local + + # Configure esbuild (the version is required) + config :esbuild, version: "0.25.4" + + # Configures Elixir's Logger + config :logger, :default_formatter, + format: "$time $metadata[$level] $message\\n", + metadata: [:request_id] + + # Use Jason for JSON parsing in Phoenix + config :phoenix, :json_library, Jason + + # Import environment specific config. This must remain at the bottom + # of this file so it overrides the configuration defined above. + import_config "\#{config_env()}.exs" + """, + """ + # This file is responsible for configuring your application + # and its dependencies with the aid of the Config module. + # + # This configuration file is loaded before any dependency and + # is restricted to this project. + + # General application configuration + import Config + + # Configure esbuild (the version is required) + config :esbuild, version: "0.25.4" + + # Configures Elixir's Logger + config :logger, :default_formatter, + format: "$time $metadata[$level] $message\\n", + metadata: [:request_id] + + # Use Jason for JSON parsing in Phoenix + config :phoenix, :json_library, Jason + + # Configures the mailer + # + # By default it uses the "Local" adapter which stores the emails + # locally. You can see the emails in your browser, at "/dev/mailbox". + # + # For production it's recommended to configure a different adapter + # at the `config/runtime.exs`. + config :phx_new, PhxNew.Mailer, adapter: Swoosh.Adapters.Local + + # Configures the endpoint + config :phx_new, PhxNewWeb.Endpoint, + url: [host: "localhost"], + adapter: Bandit.PhoenixAdapter, + pubsub_server: PhxNew.PubSub, + live_view: [signing_salt: "QdK6mkm/"] + + config :phx_new, + ecto_repos: [PhxNew.Repo], + generators: [timestamp_type: :utc_datetime] + + # Import environment specific config. This must remain at the bottom + # of this file so it overrides the configuration defined above. + import_config "\#{config_env()}.exs" + """ + ) + end + + test "phx.new prod.exs" do + assert_style( + """ + import Config + + # Note we also include the path to a cache manifest + # containing the digested version of static files. This + # manifest is generated by the `mix assets.deploy` task, + # which you should run after static files are built and + # before starting your production server. + config :phx_new, PhxNewWeb.Endpoint, cache_static_manifest: "priv/static/cache_manifest.json" + + # Configures Swoosh API Client + config :swoosh, api_client: Swoosh.ApiClient.Req + + # Disable Swoosh Local Memory Storage + config :swoosh, local: false + + # Do not print debug messages in production + config :logger, level: :info + + # Runtime production configuration, including reading + # of environment variables, is done on config/runtime.exs. + """, + """ + import Config + + # Do not print debug messages in production + config :logger, level: :info + + # Note we also include the path to a cache manifest + # containing the digested version of static files. This + # manifest is generated by the `mix assets.deploy` task, + # which you should run after static files are built and + # before starting your production server. + config :phx_new, PhxNewWeb.Endpoint, cache_static_manifest: "priv/static/cache_manifest.json" + + # Configures Swoosh API Client + config :swoosh, api_client: Swoosh.ApiClient.Req + + # Disable Swoosh Local Memory Storage + config :swoosh, local: false + + # Runtime production configuration, including reading + # of environment variables, is done on config/runtime.exs. + """ + ) + end end diff --git a/test/style/module_directives/alias_lifting_test.exs b/test/style/module_directives/alias_lifting_test.exs index 721228b..cc87660 100644 --- a/test/style/module_directives/alias_lifting_test.exs +++ b/test/style/module_directives/alias_lifting_test.exs @@ -402,6 +402,7 @@ defmodule Styler.Style.ModuleDirectives.AliasLiftingTest do """, """ alias A.B.C + # Foo is my fave require Foo diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index 5fe9a26..9381fec 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -426,7 +426,7 @@ defmodule Styler.Style.ModuleDirectivesTest do end describe "with comments..." do - test "moving aliases up through non-directives doesn't move comments up" do + test "moving aliases up through non-directives moves their comments with them" do assert_style( """ defmodule Foo do @@ -450,9 +450,11 @@ defmodule Styler.Style.ModuleDirectivesTest do defmodule Foo do # mdf @moduledoc false + # A alias A.A # B alias B.B + # C alias C.C # foo @@ -460,9 +462,6 @@ defmodule Styler.Style.ModuleDirectivesTest do # ok :ok end - - # C - # A end """ ) @@ -729,4 +728,23 @@ defmodule Styler.Style.ModuleDirectivesTest do ) end end + + describe "comment movement regressions" do + test "comment on hoisted import is stranded when an attribute is reordered above it" do + assert_style( + """ + @endpoint Foo + + # this comment belongs to the import + import Plug.Conn + """, + """ + # this comment belongs to the import + import Plug.Conn + + @endpoint Foo + """ + ) + end + end end diff --git a/test/style/pipes_test.exs b/test/style/pipes_test.exs index 570f5b9..f515bb6 100644 --- a/test/style/pipes_test.exs +++ b/test/style/pipes_test.exs @@ -1058,4 +1058,35 @@ defmodule Styler.Style.PipesTest do end end end + + describe "comment movement regressions" do + test "unpiping a single pipe drops an interleaved comment below the expression" do + assert_style( + """ + foo + # comment + |> bar() + """, + """ + # comment + bar(foo) + """ + ) + end + + test "unpiping a single pipe into an assignment hoists an interleaved comment above it" do + assert_style( + """ + x = + foo + # comment + |> bar() + """, + """ + # comment + x = bar(foo) + """ + ) + end + end end