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

bottlenecked · 2022-03-25T13:26:34Z

Hi all, this is a feature that I always wanted to have and now took some time to implement.

Basically when creating a new file and writing a defmod... prompt for auto-completion I wanted a likely module_name to be suggested along with completing the defmodule do .. end snippet.

This is what my attempt looks like

suggest_module_name.mov

There are some limitations I'm aware of (and probably tens of others that I'm not aware of!)

Nested module names: the users will be getting a suggestion based on the full file path instead of taking into account the parent module and basically doing nothing (let the users name their module)
Phoenix conventions are not observed. e.g with controller names the suggested module name will be MyProjectWeb.Controllers.PageController instead of MyProjectWeb.PageController

Still, do let me know if this PR is on the right track and if so also let me know if these limitations (and possibly others) should be fixed before attempting to ship something like this to the users.

Thanks for taking the time to review!

…e' snippet

sharpfun

Great stuff, here are some ideas)

sharpfun · 2022-03-25T15:53:12Z

apps/language_server/lib/language_server/providers/completion.ex

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


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

sharpfun · 2022-03-25T15:55:19Z

apps/language_server/lib/language_server/providers/completion.ex

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


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?

@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

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.

lukaszsamson · 2022-04-01T07:05:00Z

apps/language_server/lib/language_server/providers/completion.ex

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


convert URI to path. SourceFile has function for that

lukaszsamson · 2022-04-01T07:11:07Z

apps/language_server/lib/language_server/providers/completion.ex

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


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

lukaszsamson · 2022-04-01T07:13:43Z

Nice feature @bottlenecked. I'd be happy to marge it. Can you verify if it works with umbrella apps and on windows? Those two are often tricky.

…propriate module_names

bottlenecked · 2022-04-04T14:34:05Z

Thanks @lukaszsamson for the encouragement.

So I had some time to move forward with this to basically implement some of the suggestions here

use SourceFile.path_from_uri/1 to work with the path instead of the uri
umbrellas look like they're already handled by the current approach so I added a test to prevent regressions in the future when used with umbrella apps
Special cased the handling of known Phoenix folders directly under *_web

What I still haven't done is

make sure that this works on Windows (my feeling is that it will because Path.split/1 should be able to handle Windows paths just fine but we'll see)
handle files under the test/support folder. I'm not sure that there's a general rule that can be used like e.g. the top level module should always be MyProjectWeb.*. What do you think the rule should be like? Or maybe we just don't handle this case and we return the defmodule snippet without a suggested module name.
Stop traversing the path when reaching the project dir. Note: opening orphaned files from different parts of the disk into the same vscode instance might lead to unexpected behaviour, I'll see about testing that case

bottlenecked · 2022-04-04T15:22:21Z

Some more thoughts on stopping at the project_dir level. It seems to me that the way the code is written, most of the times the traversal would stop at either the lib or the test folder levels. I guess I'd expect most projects to be structured like the mix new / mix phx.new structure the generated code, so maybe it would not be as bad to traverse the path all the way to the top when the projects are not structured as expected (should be few enough?). Plus, I think it would be pretty cheap to go all the way to the top anyway even for extreme cases of 1000 levels of deep folder nesting. What do you think @lukaszsamson?

bottlenecked · 2022-04-15T09:55:33Z

I managed to test this on Windows (virtualbox VM on mac) and as ~~hoped for~~ expected Path.split/1 does the right thing on Windows paths too

windows_suggest.mov

Some plugin or setting in my editor trimmed extra spaces from the lines but in this case it cause a test to break by changing the cursor position of the auto-completion trigger

bottlenecked · 2022-04-15T11:15:29Z

In d3b624f the broken test is fixed (extra space at end of line was previously trimmed out and now restored) and also removed the debug IO.inspect expression that was previously added

noozo · 2022-05-04T09:58:53Z

Much needed. Now i can get rid of the "Insert Elixir Module name here" extension. Genius!

Any idea when a new release will come out that contains this?

jared-mackey · 2023-02-09T18:31:49Z

After having used this for a long while now, I just wanted to chime in and say thanks again for this. It is such a great addition! 🙏🏻 ❤️

Suggest an appropriate module name when auto-completing the 'defmodul…

37ffbef

…e' snippet

sharpfun reviewed Mar 25, 2022

View reviewed changes

lukaszsamson reviewed Apr 1, 2022

View reviewed changes

bottlenecked added 3 commits April 1, 2022 15:10

use processed file_path instead of raw file:// uri for determining ap…

3935a16

…propriate module_names

add umbrella_app test to guard against future regressions

036901c

special case common Phoenix folders when suggesting module names

dd6c054

fix broken test

d3b624f

Some plugin or setting in my editor trimmed extra spaces from the lines but in this case it cause a test to break by changing the cursor position of the auto-completion trigger

lukaszsamson approved these changes Apr 19, 2022

View reviewed changes

lukaszsamson merged commit 7f37d59 into elixir-lsp:master Apr 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bottlenecked commented Mar 25, 2022

sharpfun left a comment •

edited

Loading

sharpfun Mar 25, 2022

sharpfun Mar 25, 2022

bottlenecked Mar 28, 2022

lukaszsamson Apr 1, 2022

lukaszsamson Apr 1, 2022

lukaszsamson Apr 1, 2022

lukaszsamson commented Apr 1, 2022

bottlenecked commented Apr 4, 2022 •

edited

Loading

bottlenecked commented Apr 4, 2022

bottlenecked commented Apr 15, 2022

bottlenecked commented Apr 15, 2022 •

edited

Loading

noozo commented May 4, 2022 •

edited

Loading

jared-mackey commented Feb 9, 2023

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

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

Conversation

bottlenecked commented Mar 25, 2022

sharpfun left a comment • edited Loading

Choose a reason for hiding this comment

sharpfun Mar 25, 2022

Choose a reason for hiding this comment

sharpfun Mar 25, 2022

Choose a reason for hiding this comment

bottlenecked Mar 28, 2022

Choose a reason for hiding this comment

lukaszsamson Apr 1, 2022

Choose a reason for hiding this comment

lukaszsamson Apr 1, 2022

Choose a reason for hiding this comment

lukaszsamson Apr 1, 2022

Choose a reason for hiding this comment

lukaszsamson commented Apr 1, 2022

bottlenecked commented Apr 4, 2022 • edited Loading

bottlenecked commented Apr 4, 2022

bottlenecked commented Apr 15, 2022

bottlenecked commented Apr 15, 2022 • edited Loading

noozo commented May 4, 2022 • edited Loading

jared-mackey commented Feb 9, 2023

sharpfun left a comment •

edited

Loading

bottlenecked commented Apr 4, 2022 •

edited

Loading

bottlenecked commented Apr 15, 2022 •

edited

Loading

noozo commented May 4, 2022 •

edited

Loading