Skip to content

Commit 053539d

Browse files
committed
Check .formatter.exs :inputs relative to its directory
When dealing with `.formatter.exs` files that are in subdirectories of the main project, the behaviour was incorrect: #361 And also caused performance problems: #381 A good example is a project I currently work on. We have a top-level `.formatter.exs`, as well as a `test/.formatter.exs`. The one in `test/` has the `:inputs` option set to `["**/*.{ex,exs}"]`. Before this commit, should_format?/3 would take this pattern and concatenate it with the project directory. This would result in Path.wildcard/2 globbing for all source files in the entire project, rather than just under test/. This causes formatting to take about 5 seconds on our project. This commit fixes the problem by taking the file to be formatted, and finding the `.formatter.exs` file that exists in the closest parent directory (so long as that directory is still within the project root). We then perform the Path.wildcard/2 relative to that directory. I’ve removed the hack that was in place to support umbrella projects, because this is a more general solution to the problem. On our project, I am now able to format a file in 100ms or so, which isn’t a bad speed-up!
1 parent 5eac30c commit 053539d

File tree

7 files changed

+48
-13
lines changed

7 files changed

+48
-13
lines changed

apps/language_server/lib/language_server/providers/formatting.ex

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,20 +55,33 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do
5555

5656
defp should_format?(file_uri, project_dir, inputs) when is_list(inputs) do
5757
file_path = file_uri |> SourceFile.abs_path_from_uri()
58+
formatter_dir = find_formatter_dir(project_dir, Path.dirname(file_path))
5859

5960
inputs
60-
|> Stream.flat_map(fn glob ->
61-
[
62-
Path.join([project_dir, glob]),
63-
Path.join([project_dir, "apps", "*", glob])
64-
]
65-
end)
66-
|> Stream.flat_map(&Path.wildcard(&1, match_dot: true))
61+
|> Stream.flat_map(&Path.wildcard(Path.join(formatter_dir, &1), match_dot: true))
6762
|> Enum.any?(&(file_path == &1))
6863
end
6964

7065
defp should_format?(_file_uri, _project_dir, _inputs), do: true
7166

67+
# Finds the deepest directory that contains file_path, that also contains a
68+
# .formatter.exs. It's possible, though unlikely, that the .formatter.exs we
69+
# find is not actually linked to the project_dir via the :subdirectories
70+
# option in the top-level .formatter.exs. Currently, that edge case is
71+
# glossed over.
72+
defp find_formatter_dir(project_dir, dir) do
73+
cond do
74+
dir == project_dir ->
75+
project_dir
76+
77+
Path.join(dir, ".formatter.exs") |> File.exists?() ->
78+
dir
79+
80+
true ->
81+
find_formatter_dir(project_dir, Path.dirname(dir))
82+
end
83+
end
84+
7285
defp myers_diff_to_text_edits(myers_diff) do
7386
myers_diff_to_text_edits(myers_diff, {0, 0}, [])
7487
end

apps/language_server/test/fixtures/formatter/.formatter.exs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
line_length: 20,
33
subdirectories: [
44
"lib",
5-
"test"
6-
]
5+
"test",
6+
"symlink",
7+
"apps/*"
8+
],
9+
inputs: ["*.ex"]
710
]
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
[
2+
inputs: ["foo.ex"]
3+
]
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
test
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
[
2-
inputs: ["**/*.exs"]
2+
inputs: ["*.exs"]
33
]

apps/language_server/test/providers/formatting_test.exs

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -481,18 +481,29 @@ defmodule ElixirLS.LanguageServer.Providers.FormattingTest do
481481

482482
assert_formatted("file.ex", project_dir)
483483

484-
# test/.formatter.exs has [inputs: ["**/*.exs"]]
484+
# test/.formatter.exs has [inputs: ["*.exs"]]
485485
assert_formatted("test/file.exs", project_dir)
486486
refute_formatted("test/file.ex", project_dir)
487+
488+
assert_formatted("symlink/file.exs", project_dir)
489+
refute_formatted("symlink/file.ex", project_dir)
490+
491+
File.mkdir!("#{project_dir}/test/foo")
492+
refute_formatted("test/foo/file.ex", project_dir)
493+
494+
# apps/foo/bar/.formatter.exs has [inputs: ["foo.ex"]]
495+
assert_formatted("apps/foo/foo.ex", project_dir)
496+
refute_formatted("apps/foo/bar.ex", project_dir)
497+
refute_formatted("apps/foo.ex", project_dir)
487498
end)
488499
end
489500

490501
def assert_formatted(path, project_dir) do
491-
assert {:ok, [%{}]} = format(path, project_dir)
502+
assert match?({:ok, [%{}]}, format(path, project_dir)), "expected '#{path}' to be formatted"
492503
end
493504

494505
def refute_formatted(path, project_dir) do
495-
assert {:ok, []} = format(path, project_dir)
506+
assert match?({:ok, []}, format(path, project_dir)), "expected '#{path}' not to be formatted"
496507
end
497508

498509
defp format(path, project_dir) do

apps/language_server/test/server_test.exs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -899,6 +899,10 @@ defmodule ElixirLS.LanguageServer.ServerTest do
899899
in_fixture(__DIR__, "formatter", fn ->
900900
uri = Path.join([root_uri(), "file.ex"])
901901

902+
uri
903+
|> SourceFile.abs_path_from_uri()
904+
|> File.write!("")
905+
902906
code = """
903907
defmodule MyModule do
904908
def my_fn do

0 commit comments

Comments
 (0)