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 umbrella support for "Find References" #20

Closed
msaraiva opened this issue Jul 29, 2019 · 3 comments
Closed

Add umbrella support for "Find References" #20

msaraiva opened this issue Jul 29, 2019 · 3 comments

Comments

@msaraiva
Copy link
Collaborator

I noticed that ElixirLS does not use ElixirSense.references/3. That means it will not benefit from the latest changes that provide accurate information on function calls references, including the column range and a bunch of other improvements. Those changes are absolutely necessary for the refactoring feature.

@balduncle, maybe this was the problem you were facing?

@JakeBecker be aware that the current ElixirLS implementation is not compatible with the latest version of ElixirSense. There's at least one breaking change regarding ElixirLS.LanguageServer.Providers.References calling functions from the ElixirSense.Core.Introspection module. Usually, in order to avoid more incompatibilities, clients should try as much as possible to only call functions from the public ElixirSense module API, which is the official API.

As far as I could see, the only thing that was implemented in the ElixirLS version that was not implemented in ElixirSense, is the support for references in umbrella projects here.

So, I believe we should, in ElxirSense:

  • Add umbrella support for "Find references"
  • Document that only functions from the ElixirSense module are part of the public API.

In ElixirLS:

  • Use ElixirSense.references/3 instead of own implementation
  • Search for other modules that are using ElixirSense non-public API and check if they still work.

@axelson and @lukaszsamson I'm not sure if elixir-lsp/elixir-ls is in sync with the upstream version but I believe it would present the same problem.

@justindotpub
Copy link

@msaraiva yes it certainly was and I didn't have the time to dig into the code and give you a well formulated assessment of how the new functionality was working. Sorry for the delay.

@JakeBecker
Copy link
Contributor

Cool, this is good to know @msaraiva . I'll update ElixirLS to work with the latest Elixir Sense. Column info is going to be super helpful 👍

@msaraiva msaraiva mentioned this issue Aug 7, 2019
@msaraiva
Copy link
Collaborator Author

Closed by #46

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

No branches or pull requests

3 participants