From 6c2b5c9ef952f1b3f16162422cb2ea2cda26a5d2 Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 22 Jun 2023 14:55:50 -0600 Subject: [PATCH 1/2] Fix ModuleDirectives comments (Closes #29) Co-authored-by: Greg Mefford --- lib/style/module_directives.ex | 56 ++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index d7507a34..c6fd911c 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -150,7 +150,7 @@ defmodule Styler.Style.ModuleDirectives do requires = expand_and_sort(directives[:require] || []) directives = - Enum.concat([ + [ shortdocs, moduledocs, behaviours, @@ -158,7 +158,9 @@ defmodule Styler.Style.ModuleDirectives do imports, aliases, requires - ]) + ] + |> Enum.concat() + |> fix_line_numbers() cond do Enum.empty?(directives) -> @@ -178,6 +180,56 @@ defmodule Styler.Style.ModuleDirectives do end end + # This is the step that ensures that comments don't get wrecked as part of us moving AST nodes willy-nilly. + # + # For example, given document + # + # 1: defmodule ... + # 2: alias B + # 3: # hi + # 4: # this is foo + # 5: def foo ... + # 6: alias A + # + # Moving the ast node for alias A would put line 6 before line 2 in the AST. + # Elixir's document algebra would then encounter "line 6" and immediately dump all comments with line < 6, + # 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 fixes that error by ensuring the following property: + # A given node of AST cannot have a line number greater than the next AST node. + # Et voila! Comments behave much better. + defp fix_line_numbers(directives, acc \\ []) + + defp fix_line_numbers([{_, this_meta, _} = this, {_, next_meta, _} = next | rest], acc) do + next_line = next_meta[:line] + this_line = this_meta[:line] + + this = + if this_line > next_line do + Style.update_all_meta(this, fn meta -> + meta + |> Keyword.replace(:line, next_line) + |> Keyword.replace(:closing, line: next_line) + |> Keyword.replace(:last, line: next_line) + end) + else + this + end + + fix_line_numbers([next | rest], [this | acc]) + end + + defp fix_line_numbers([last], acc), do: Enum.reverse([last | acc]) + defp fix_line_numbers([], []), do: [] + defp expand_and_sort(directives) do # sorting is done with `downcase` to match Credo directives From 26127bc51a56fe999f491699a0206a6f6bc2f93b Mon Sep 17 00:00:00 2001 From: Matt Enlow Date: Thu, 22 Jun 2023 15:19:32 -0600 Subject: [PATCH 2/2] add a test --- lib/style/module_directives.ex | 1 + test/style/module_directives_test.exs | 32 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/lib/style/module_directives.ex b/lib/style/module_directives.ex index c6fd911c..c1132705 100644 --- a/lib/style/module_directives.ex +++ b/lib/style/module_directives.ex @@ -228,6 +228,7 @@ defmodule Styler.Style.ModuleDirectives do end defp fix_line_numbers([last], acc), do: Enum.reverse([last | acc]) + defp fix_line_numbers([], []), do: [] defp expand_and_sort(directives) do diff --git a/test/style/module_directives_test.exs b/test/style/module_directives_test.exs index b2188e88..7abc206e 100644 --- a/test/style/module_directives_test.exs +++ b/test/style/module_directives_test.exs @@ -349,4 +349,36 @@ defmodule Styler.Style.ModuleDirectivesTest do """) end end + + describe "with comments..." do + test "moving aliases up through non-directives doesn't move comments up" do + assert_style( + """ + defmodule Foo do + alias B + # hi + # this is foo + def foo do + # i promise it's ok! + :ok + end + alias A + end + """, + """ + defmodule Foo do + @moduledoc false + alias A + alias B + # hi + # this is foo + def foo do + # i promise it's ok! + :ok + end + end + """ + ) + end + end end