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

Add replacing unknown remote function to code actions #443

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

sheldak
Copy link
Contributor

@sheldak sheldak commented Oct 24, 2023

I've prepared that PR to propose extending code actions by replacing unknown remote functions (I did a similar thing in that PR for ElixirLS). For example, the following code:

defmodule Example do
  def foo do
    var = Enum.counts([2, 3])
  end
end

contains an unknown function - counts/1. After this change, we would get 2 possible quick fixes:

Rename to concat
Rename to count

that rewrite our code to:

defmodule Example do
  def foo do
    var = Enum.concat([2, 3])
  end
end

or

defmodule Example do
  def foo do
    var = Enum.count([2, 3])
  end
end

respectively.

In our example, code actions were generated using String.jaro_distance/2 with a threshold of 0.77 to create at most 5 suggestions (similar to did you mean functionality).

@zachallaun
Copy link
Collaborator

Thanks for the contribution! I'll look more carefully at this soon but a couple of early points:

  1. CI is failing because of a Credo warning -- you can just run mix credo to find/fix.
  2. I'd prefer not to generate suggestions by parsing the Elixir error. This seems brittle and prone to breakage if Elixir ever changes how these suggestions are presented. Instead, we should generate the suggestions ourselves using the same mechanism Elixir uses: String.jaro_distance/2. You can get a list of functions/macros from a module with module.__info__/1, and it's easy enough to compare from there.
  3. We'd like to move away from using ElixirSense. Look for Lexical.Ast.expand_alises as a replacement for what I think you're currently using it for.

@scohen
Copy link
Collaborator

scohen commented Oct 24, 2023

Also, thank you for writing so many tests!

@scohen
Copy link
Collaborator

scohen commented Oct 24, 2023

Also, looking at all the work you had to do to add another action makes me think we can improve the process by coming up with a simpler API for code actions, possibly pushing the bulk of their logic into remote_control, so that we only need to call a single function in remote_control to get all the relevant code actions.

@sheldak
Copy link
Contributor Author

sheldak commented Oct 25, 2023

Thanks for a quick response!

  1. I'd prefer not to generate suggestions by parsing the Elixir error. This seems brittle and prone to breakage if Elixir ever changes how these suggestions are presented. Instead, we should generate the suggestions ourselves using the same mechanism Elixir uses: String.jaro_distance/2. You can get a list of functions/macros from a module with module.__info__/1, and it's easy enough to compare from there.

That makes sense. However, I have an issue with module.__info__/1 - I cannot use it since the module is not available from Lexical (we only have the module as a document). I did something similar previously here but it required ElixirSense.

  1. We'd like to move away from using ElixirSense. Look for Lexical.Ast.expand_alises as a replacement for what I think you're currently using it for.

That should work

@scohen
Copy link
Collaborator

scohen commented Oct 25, 2023

That makes sense. However, I have an issue with module.info/1 - I cannot use it since the module is not available from Lexical

I think you need to do this in the project node (the remote_control app). Then you should have access to the module.

@scohen
Copy link
Collaborator

scohen commented Oct 27, 2023

@sheldak Check out #453

@sheldak
Copy link
Contributor Author

sheldak commented Oct 31, 2023

@sheldak Check out #453

Thanks, I will rebase to that. I will be able to come back to this PR next week.

@scohen
Copy link
Collaborator

scohen commented Nov 2, 2023

@sheldak The PR has been merged, I think this PR is will now be much simpler to implement and test.

@sheldak
Copy link
Contributor Author

sheldak commented Nov 7, 2023

@zachallaun @scohen All changes made!

@scohen
Copy link
Collaborator

scohen commented Nov 7, 2023

@sheldak what did you think of the changes?

Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

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

Looks pretty good. The lion's share of the file changes here are fixtures for testing in the server app.

I have some minor comments, but the approach looks solid to me.

@sheldak
Copy link
Contributor Author

sheldak commented Nov 8, 2023

@sheldak what did you think of the changes?

It was a big improvement. They made everything more concise and I particularly liked not passing all these arguments through several functions to do only code modification in the remote control.

@scohen
Copy link
Collaborator

scohen commented Nov 8, 2023

Looking at your Pr, I concur. I just wanted to make sure you felt the same.

@scohen
Copy link
Collaborator

scohen commented Nov 21, 2023

@sheldak looks like you're going to need to rebase.

@scohen
Copy link
Collaborator

scohen commented Nov 21, 2023

Never mind, i'll do this for you.

@scohen scohen merged commit d085f34 into lexical-lsp:main Nov 21, 2023
1 of 7 checks passed
@sheldak sheldak deleted the replace-remote-function branch November 22, 2023 14:11
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.

3 participants