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

Suggest an appropriate module name with the 'defmodule' snippet #684

Merged
merged 5 commits into from
Apr 19, 2022
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
79 changes: 78 additions & 1 deletion apps/language_server/lib/language_server/providers/completion.ex
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,9 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do
completion
end

if snippet = snippet_for({origin, name}, context) do
file_path = Keyword.get(options, :file_path)

if snippet = snippet_for({origin, name}, Map.put(context, :file_path, file_path)) do
%{completion | insert_text: snippet, kind: :snippet, label: name}
else
completion
Expand All @@ -576,6 +578,12 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do
nil
end

defp snippet_for({"Kernel", "defmodule"}, %{file_path: file_path}) when is_binary(file_path) do
# In a mix project the file_path can be something like "/some/code/path/project/lib/project/sub_path/my_file.ex"
# so we'll try to guess the appropriate module name from the path
"defmodule #{suggest_module_name(file_path)}$1 do\n\t$0\nend"
end

defp snippet_for(key, %{pipe_before?: true}) do
# Get pipe-friendly version of snippet if available, otherwise fallback to standard
Map.get(@pipe_func_snippets, key) || Map.get(@func_snippets, key)
Expand All @@ -593,6 +601,75 @@ defmodule ElixirLS.LanguageServer.Providers.Completion do
end
end

def suggest_module_name(file_path) when is_binary(file_path) do
file_path
|> Path.split()
|> Enum.reverse()
|> do_suggest_module_name()
end

defp do_suggest_module_name([]), do: nil

defp do_suggest_module_name([filename | reversed_path]) do
filename
|> String.split(".")
|> case do
[file, "ex"] ->
do_suggest_module_name(reversed_path, [file], topmost_parent: "lib")

Choose a reason for hiding this comment

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

Usually phoenix projects have test/support folder that have these files:

channel_case.ex
conn_case.ex
data_case.ex

Factories or other helpers are created in that folder.
Based on file extension topmost_parent will be "lib" which would be incorrect


[file, "exs"] ->
if String.ends_with?(file, "_test") do
do_suggest_module_name(reversed_path, [file], topmost_parent: "test")

Choose a reason for hiding this comment

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

I've seen code with tests in lib folder, it was assumed, that it gives better test coverage.
Maybe there is a way to get root folder of the project?
Umbrella projects will definitely complicate root folder solution :/
Or both "lib" and "test" could be used as topmost_parent.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sharpfun Thanks for the suggestions, but I'm hesitant to go any further at this point before anyone of the maintainers chimes in because it might be the wrong way to go about it

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't make this work for everyone. I'd aim for covering basic mix project and phoenix app as those are most commonly used.

else
nil
end

_otherwise ->
nil
end
end

defp do_suggest_module_name([dir | _rest], module_name_acc, topmost_parent: topmost)
when dir == topmost do
module_name_acc
|> Enum.map(&Macro.camelize/1)
|> Enum.join(".")
end

defp do_suggest_module_name(
[probable_phoenix_dir | [project_web_dir | _] = rest],
module_name_acc,
opts
)
when probable_phoenix_dir in [
"controllers",
"views",
"channels",
"plugs",
"endpoints",
"sockets"
] do
if String.ends_with?(project_web_dir, "_web") do
# by convention Phoenix doesn't use these folders as part of the module names
# for modules located inside them, so we'll try to do the same
do_suggest_module_name(rest, module_name_acc, opts)
else
# when not directly under the *_web folder however then we should make the folder
# part of the module's name
do_suggest_module_name(rest, [probable_phoenix_dir | module_name_acc], opts)
end
end

defp do_suggest_module_name([dir_name | rest], module_name_acc, opts) do
do_suggest_module_name(rest, [dir_name | module_name_acc], opts)
end

defp do_suggest_module_name([], _module_name_acc, _opts) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it optimal to go all the way up to / or C:\? We could stop at workdir level.

# we went all the way up without ever encountering a 'lib' or a 'test' folder
# so we ignore the accumulated module name because it's probably wrong/useless
nil
end

def function_snippet(name, args, arity, opts) do
snippets_supported? = Keyword.get(opts, :snippets_supported, false)
trigger_signature? = Keyword.get(opts, :trigger_signature?, false)
Expand Down
3 changes: 2 additions & 1 deletion apps/language_server/lib/language_server/server.ex
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,8 @@ defmodule ElixirLS.LanguageServer.Server do
tags_supported: tags_supported,
signature_help_supported: signature_help_supported,
locals_without_parens: locals_without_parens,
signature_after_complete: signature_after_complete
signature_after_complete: signature_after_complete,
file_path: SourceFile.path_from_uri(uri)
)
end

Expand Down
131 changes: 131 additions & 0 deletions apps/language_server/test/providers/completion_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,62 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do
"""
}
end

test "will suggest defmodule with module_name snippet when file path matches **/lib/**/*.ex" do
text = """
defmod
# ^
"""

{line, char} = {0, 6}

TestUtils.assert_has_cursor_char(text, line, char)

assert {:ok, %{"items" => [first | _] = _items}} =
Completion.completion(
text,
line,
char,
@supports
|> Keyword.put(
:file_path,
"/some/path/my_project/lib/my_project/sub_folder/my_file.ex"
)
)

assert %{
"label" => "defmodule",
"insertText" => "defmodule MyProject.SubFolder.MyFile$1 do\n\t$0\nend"
} = first
end

test "will suggest defmodule without module_name snippet when file path does not match expected patterns" do
text = """
defmod
# ^
"""

{line, char} = {0, 6}

TestUtils.assert_has_cursor_char(text, line, char)

assert {:ok, %{"items" => [first | _] = _items}} =
Completion.completion(
text,
line,
char,
@supports
|> Keyword.put(
:file_path,
"/some/path/my_project/lib/my_project/sub_folder/my_file.heex"
)
)

assert %{
"label" => "defmodule",
"insertText" => "defmodule $1 do\n\t$0\nend"
} = first
end
end

describe "generic suggestions" do
Expand Down Expand Up @@ -1053,4 +1109,79 @@ defmodule ElixirLS.LanguageServer.Providers.CompletionTest do
assert insert_text =~ "if do\n\t"
end
end

describe "suggest_module_name/1" do
import Completion, only: [suggest_module_name: 1]

test "returns nil if current file_path is empty" do
assert nil == suggest_module_name("")
end

test "returns nil if current file is not an .ex file" do
assert nil == suggest_module_name("some/path/lib/dir/file.heex")
end

test "returns nil if current file is an .ex file but no lib folder exists in path" do
assert nil == suggest_module_name("some/path/not_lib/dir/file.ex")
end

test "returns nil if current file is an *_test.exs file but no test folder exists in path" do
assert nil == suggest_module_name("some/path/not_test/dir/file_test.exs")
end

test "returns an appropriate suggestion if file directly under lib" do
assert "MyProject" == suggest_module_name("some/path/my_project/lib/my_project.ex")
end

test "returns an appropriate suggestion if file arbitrarily nested under lib/" do
assert "MyProject.Foo.Bar.Baz.MyFile" =
suggest_module_name("some/path/my_project/lib/my_project/foo/bar/baz/my_file.ex")
end

test "returns an appropriate suggestion if file directly under test/" do
assert "MyProjectTest" ==
suggest_module_name("some/path/my_project/test/my_project_test.exs")
end

test "returns an appropriate suggestion if file arbitrarily nested under test" do
assert "MyProject.Foo.Bar.Baz.MyFileTest" ==
suggest_module_name(
"some/path/my_project/test/my_project/foo/bar/baz/my_file_test.exs"
)
end

test "returns an appropriate suggestion if file is part of an umbrella project" do
assert "MySubApp.Foo.Bar.Baz" ==
suggest_module_name(
"some/path/my_umbrella_project/apps/my_sub_app/lib/my_sub_app/foo/bar/baz.ex"
)
end

test "returns appropriate suggestions for modules nested under known phoenix dirs" do
[
{"MyProjectWeb.MyController", "controllers/my_controller.ex"},
{"MyProjectWeb.MyPlug", "plugs/my_plug.ex"},
{"MyProjectWeb.MyView", "views/my_view.ex"},
{"MyProjectWeb.MyChannel", "channels/my_channel.ex"},
{"MyProjectWeb.MyEndpoint", "endpoints/my_endpoint.ex"},
{"MyProjectWeb.MySocket", "sockets/my_socket.ex"}
]
|> Enum.each(fn {expected_module_name, partial_path} ->
path = "some/path/my_project/lib/my_project_web/#{partial_path}"
assert expected_module_name == suggest_module_name(path)
end)
end

test "uses known Phoenix dirs as part of a module's name if these are not located directly beneath the *_web folder" do
assert "MyProject.Controllers.MyController" ==
suggest_module_name(
"some/path/my_project/lib/my_project/controllers/my_controller.ex"
)

assert "MyProjectWeb.SomeNestedDir.Controllers.MyController" ==
suggest_module_name(
"some/path/my_project/lib/my_project_web/some_nested_dir/controllers/my_controller.ex"
)
end
end
end