Skip to content

Commit 7580f18

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 7580f18

File tree

5 files changed

+56
-15
lines changed

5 files changed

+56
-15
lines changed

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

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,19 +56,47 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do
5656
defp should_format?(file_uri, project_dir, inputs) when is_list(inputs) do
5757
file_path = file_uri |> SourceFile.abs_path_from_uri()
5858

59-
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))
67-
|> Enum.any?(&(file_path == &1))
59+
if File.exists?(file_path) do
60+
formatter_dir = find_formatter_dir(project_dir, file_path)
61+
62+
inputs
63+
|> Stream.flat_map(&Path.wildcard(Path.join([formatter_dir, &1]), match_dot: true))
64+
|> Enum.any?(&(file_path == &1))
65+
else
66+
# If the file doesn't actually exist on disk, we can't currently check
67+
# whether we should format it via Path.wildcard/2, as that operation is
68+
# dependent on the filesystem. So just assume that we should.
69+
true
70+
end
6871
end
6972

7073
defp should_format?(_file_uri, _project_dir, _inputs), do: true
7174

75+
# Finds the deepest directory that contains file_path, that also contains a
76+
# .formatter.exs. It's possible, though unlikely, that the .formatter.exs we
77+
# find is not actually linked to the project_dir via the :subdirectories
78+
# option in the top-level .formatter.exs. Currently, that edge case is
79+
# glossed over.
80+
defp find_formatter_dir(project_dir, file_path) do
81+
parts = Path.split(file_path)
82+
project_parts = Path.split(project_dir)
83+
84+
unless List.starts_with?(parts, project_parts) do
85+
raise ArgumentError, "path '#{file_path}' is not within '#{project_dir}'"
86+
end
87+
88+
parts = Enum.drop(parts, length(project_parts))
89+
90+
{subdirs, _} =
91+
for part <- parts, reduce: {[], project_dir} do
92+
{dirs, prev_dir} ->
93+
dir = Path.join([prev_dir, part])
94+
{[dir | dirs], dir}
95+
end
96+
97+
Enum.find(subdirs, &File.exists?(Path.join(&1, ".formatter.exs"))) || project_dir
98+
end
99+
72100
defp myers_diff_to_text_edits(myers_diff) do
73101
myers_diff_to_text_edits(myers_diff, {0, 0}, [])
74102
end

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
line_length: 20,
33
subdirectories: [
44
"lib",
5-
"test"
6-
]
5+
"test",
6+
"apps/*"
7+
],
8+
inputs: ["*.ex"]
79
]
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 & 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: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -481,18 +481,26 @@ 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+
File.mkdir!("#{project_dir}/test/foo")
489+
refute_formatted("test/foo/file.ex", project_dir)
490+
491+
# apps/foo/bar/.formatter.exs has [inputs: ["foo.ex"]]
492+
assert_formatted("apps/foo/foo.ex", project_dir)
493+
refute_formatted("apps/foo/bar.ex", project_dir)
494+
refute_formatted("apps/foo.ex", project_dir)
487495
end)
488496
end
489497

490498
def assert_formatted(path, project_dir) do
491-
assert {:ok, [%{}]} = format(path, project_dir)
499+
assert match?({:ok, [%{}]}, format(path, project_dir)), "expected '#{path}' to be formatted"
492500
end
493501

494502
def refute_formatted(path, project_dir) do
495-
assert {:ok, []} = format(path, project_dir)
503+
assert match?({:ok, []}, format(path, project_dir)), "expected '#{path}' not to be formatted"
496504
end
497505

498506
defp format(path, project_dir) do

0 commit comments

Comments
 (0)