Skip to content

fix(formatter): fix small issue where there was only one newline added #13867

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix(formatter): fix small issue where there was only one newline added #13867

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
732cbd7
fix(formatter): fix small issue where there was only one newline added
pdgonzalez872 Sep 28, 2024
3e2692f
Try to add above, not below.
pdgonzalez872 Sep 30, 2024
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
12 changes: 10 additions & 2 deletions lib/elixir/lib/code/formatter.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,8 @@ defmodule Code.Formatter do
end

defp block_args_to_algebra(args, min_line, max_line, state) do
quoted_to_algebra = fn {kind, meta, _} = arg, _args, state ->
newlines = meta[:end_of_expression][:newlines] || 1
quoted_to_algebra = fn {kind, meta, _} = arg, args, state ->
newlines = maybe_add_newlines({kind, args}, meta)
{doc, state} = quoted_to_algebra(arg, :block, state)
{{doc, block_next_line(kind), newlines}, state}
end
Expand All @@ -656,6 +656,14 @@ defmodule Code.Formatter do
defp block_next_line(:@), do: @empty
defp block_next_line(_), do: break("")

defp maybe_add_newlines({:def, [{:@, _, _} = _next | _]}, _meta) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not look at def, because we can have def, defp, defmacro, defmacrop... and projects can define defn, defnp, etc. We do have some code today that skips adding new lines if they are module attributes but perhaps the code should only avoid adding lines below, not lines up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes a lot of sense.

I'll try to play around with this in the next couple of days, but I won't be upset if you solve this in 2 minutes :)

No hard feelings :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim, seems closer now, but likely a bit far from the optimal solution.

I kinda wanted to offer a devil's advocate here, to the issue we put together 😂. After reading and working on the code a little bit, I came to appreciate the user's choice. I didn't think of this when seeing the issue for the first time mostly because I had not seen/worked with/knew how the formatter works.

I like how the formatter respect's the user's choice first. The crux to this devil's advocate argument is that the original code "wanted" to save space by doing the shorthard def.

If space was not an issue, the code would have used a multiline def, which the formatter already handles:

# pre-format
defmodule Example do
  @impl FooA
  def a() do
    :something
  end
  @impl FooB
  def b() do
    :something_else
  end
end

# post-format
defmodule Example do
  @impl FooA
  def a() do
    :something
  end

  @impl FooB
  def b() do
    :something_else
  end
end

Even after writing the above I'm more convinced that this is:

  • not applicable
  • likely a doc adjustment to document the user's choice rather than a behavior change

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, looking at all the formatting changes in this repo highlighted by the CI, this seems to have a much bigger impact than just the original example, and the user's choice perspective is a good point.

@newlines
end

defp maybe_add_newlines(_, meta) do
meta[:end_of_expression][:newlines] || 1
end

## Operators

defp maybe_unary_op_to_algebra(fun, meta, args, context, state) do
Expand Down
50 changes: 50 additions & 0 deletions lib/elixir/test/elixir/code_formatter/general_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,56 @@ defmodule Code.Formatter.GeneralTest do
assert_format bad, good
end

test "with def followed by module attribute" do
bad = """
defmodule Example do
@impl FooA
def a(), do: :something
@impl FooB
def b() do
:something_else
end
end
"""

good = """
defmodule Example do
@impl FooA
def a(), do: :something

@impl FooB
def b() do
:something_else
end
end
"""

assert_format bad, good
end

test "with def followed by def" do
bad = """
defmodule Example do
def a(), do: :something
def b() do
:something_else
end
end
"""

good = """
defmodule Example do
def a(), do: :something

def b() do
:something_else
end
end
"""

assert_format bad, good
end

test "with multiple defs" do
assert_same """
def foo(:one), do: 1
Expand Down