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

Fix ModuleDirectives comments #53

Merged
merged 2 commits into from
Jun 22, 2023
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
57 changes: 55 additions & 2 deletions lib/style/module_directives.ex
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,17 @@ defmodule Styler.Style.ModuleDirectives do
requires = expand_and_sort(directives[:require] || [])

directives =
Enum.concat([
[
shortdocs,
moduledocs,
behaviours,
uses,
imports,
aliases,
requires
])
]
|> Enum.concat()
|> fix_line_numbers()

cond do
Enum.empty?(directives) ->
Expand All @@ -178,6 +180,57 @@ 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
Expand Down
32 changes: 32 additions & 0 deletions test/style/module_directives_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -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