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

feat: add alias refactor workspace command #386

Merged
merged 22 commits into from
Apr 19, 2024

Conversation

NJichev
Copy link
Collaborator

@NJichev NJichev commented Feb 28, 2024

This adds an alias refactor workspace command to NextLS.

Your cursor should be at the target module that you wish to alias. It will insert the alias at the top of the nearest defmodule definition scoping the refactor only to the current module instead of the whole file.

Bar.to_list(map)
end

def bar do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why Macro.to_string picks to do/end block the one-liner. The ast from a one liner and a do/end block is the same so that's probably why. I might be missing some context data in one of the cases when testing in iex.

lib/next_ls.ex Outdated
position = arguments["position"]
text = lsp.assigns.documents[uri]

NextLS.Commands.Alias.refactor(%{
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets just call the "main method" function of these modules "run"

@mhanberg
Copy link
Collaborator

mhanberg commented Mar 8, 2024

@NJichev is this ready for review?

@NJichev NJichev force-pushed the add-alias-refactor-command branch from 2fa58c5 to 36d1030 Compare March 8, 2024 11:04
@NJichev
Copy link
Collaborator Author

NJichev commented Mar 8, 2024

It's ready for review(s) now

@mhanberg
Copy link
Collaborator

mhanberg commented Mar 8, 2024

So I tried it out and was seeing some exceptions thrown.

To use AST with Sourceror, you need to pass the same literal_encoder as we're doing in the pipe command. This is because literals don't contain metadata (as they are literals), so we use the literal encoder to turn them into normal ast nodes that have the metadata.

I made that change and now the tests aren't passing, so you'll want to make that change and then get the tests passing again.

lib/foo.ex Outdated
@@ -0,0 +1,22 @@
defmodule Foo do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this file accidentally added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep my bad

@mhanberg
Copy link
Collaborator

Ready for another review?

@NJichev
Copy link
Collaborator Author

NJichev commented Mar 18, 2024

Yeah, it should be good for review

@mhanberg
Copy link
Collaborator

@NJichev have you tested this in an editor? I can't seem to get it to work at all now.

@mhanberg
Copy link
Collaborator

ahh, it doesn't work with a remote call with args, like Foo.Bar.Baz.run()

@mhanberg
Copy link
Collaborator

You'll need to modify this to work with structs and also inside import Foo.Bar.Baz and require Foo.Bar.Baz

@NJichev
Copy link
Collaborator Author

NJichev commented Mar 27, 2024

Those issues should be fixed now

@mhanberg
Copy link
Collaborator

CleanShot.2024-03-27.at.15.16.57.mp4

found another bug. tried to alias this struct and it added the alias to a submodule below and didn't actually change any of the references

This adds an alias refactor workspace command to NextLS.

Your cursor should be at the target module that you wish to alias.
It will insert the alias at the of the nearest defmodule definition
scoping the refactor only to the current module instead of the whole
file.
Rework parts of the code since do/end block is a different ast node, it
wasn't matching hence crashing the old code
Also make alias refactor include the start of the module as a viable
cursor position. Fix test with bad character ending.
Also refactors the labeling on the workspace commands.
@mhanberg
Copy link
Collaborator

mhanberg commented Apr 5, 2024

found another bug. tried to alias this struct and it added the alias to a submodule below and didn't actually change any of the references

i noticed that this happens for the require module code action as well, if you get some time, please fix that in another PR as well.

@NJichev
Copy link
Collaborator Author

NJichev commented Apr 5, 2024

I think I already fixed it here, the only work left here is to update spitfire and handle the comments metadata.

So the issue was that the find nearest module did literally this, and it found the nearest defmodule instead of the defmodule context of the cursor. Unless it's another issue.

I'll take a look during the weekend and finish this PR.

@mhanberg
Copy link
Collaborator

mhanberg commented Apr 5, 2024

I think I already fixed it here, the only work left here is to update spitfire and handle the comments metadata.

So the issue was that the find nearest module did literally this, and it found the nearest defmodule instead of the defmodule context of the cursor. Unless it's another issue.

I'll take a look during the weekend and finish this PR.

oh gotcha, cool. i'll check again once we get this merged

@mhanberg
Copy link
Collaborator

Latest issue (after rebasing on main):

it seems to be putting the alias in the right module, but it seems to be duplicating some comments and shifting them into a weird spot

CleanShot.2024-04-13.at.09.29.06.mp4

The comments were duplicating since we alter the AST only for one
module. Filter only to comments within the module.
@mhanberg
Copy link
Collaborator

Okay, my previous comment seems fixed and is working well, now a new problem 😅

it appears that new line escape sequences in strings are getting replaced with actual new lines

I need to test this in vscode as well to make sure this isn't a client issue

CleanShot 2024-04-18 at 08 24 46@2x

@mhanberg mhanberg merged commit e14a611 into elixir-tools:main Apr 19, 2024
14 checks passed
@mhanberg
Copy link
Collaborator

Glorious!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants