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

allow disabling automatic alias insertion #774

Closed
LukasKnuth opened this issue Nov 17, 2022 · 17 comments · Fixed by #881
Closed

allow disabling automatic alias insertion #774

LukasKnuth opened this issue Nov 17, 2022 · 17 comments · Fixed by #881

Comments

@LukasKnuth
Copy link

Environment

  • Elixir & Erlang versions (elixir --version): Erlang/OTP 24 [erts-12.1.5], Elixir 1.13.0 (compiled with Erlang/OTP 22)
  • VSCode ElixirLS version: 0.12.0
  • Operating System Version: darwin 21.6.0
  • Editor or IDE name (e.g. Emacs/VSCode): VSCode
  • Editor Plugin/LSP Client name and version: v0.12.0

Current behavior

When looking for a Module via auto-completion, such as "Application", many items are shown to select from:

image

These suggestions all look the same. If I auto-complete the first one, it automatically adds

alias Plug.Crypto.Application

to the top of my file. This is not what I'm looking for.

Additionally, if I have modules inside of my Module, autocompleting these anywhere will add an alias as well, which I guess is correct but also useless:

defmodule A do
  # THIS is added when auto completing in f/0
  alias A.B

  defmodule B do
    # some stuff here
  end

  def f do
    B.something()
  end
end

I looked through the extension settings but can't find a way to disable this feature. I looked though the code in #722 (which I understand introduces this feature) but couldn't find anything either.

Expected behavior

  • Order the suggestions "shortest module path first"
    • For example, Application before Plug.Crypto.Application
  • Include the full module path into the suggestions so that you have to type the full module once, but for already aliased modules, no lookup is performed
  • OR let me deactivate the auto-alias behaviour entirely
@scohen
Copy link
Contributor

scohen commented Nov 17, 2022

+1 to this. The UX needs to be improved. I agree 100% with the first two bullet points regarding expected behavior.
FYI, here's how it looks in emacs:
Screen Shot 2022-11-17 at 9 21 29 AM

Notice that it's impossible to tell the difference between which modules you're going to alias, and it seems like the one marked implementation is actually a protocol implementation, which is almost never needed as an alias, and shouldn't get such prominent billing.

Interestingly, the actual module I would like to import is nowhere to be found in the list.

Also, it's puzzling to me why DidChange and DidChangeConfiguration appear in the list at all, since they don't start with Diag.

@mindok
Copy link

mindok commented Nov 18, 2022

+1 from me too. Spent a while figuring out why NaiveDateTime.from_iso8601! wasn't working. It had snuck in an alias Timex.NaiveDateTime. Seeing the details of the module would be great, so at least you can pick the right one - the auto-alias functionality is really handy until there are module name clashes.

@mindok
Copy link

mindok commented Nov 18, 2022

Also, it's puzzling to me why DidChange and DidChangeConfiguration appear in the list at all, since they don't start with Diag.

Fuzzy search.

@scohen
Copy link
Contributor

scohen commented Nov 18, 2022

That’s pretty darn fuzzy 😂

@lukaszsamson
Copy link
Collaborator

I think we need to display full module name if the alias is going to be inserted (or some other visual information). There's a detail field but it's not displayed by default.
Regarding protocol implementations, I think we should deprioretize them.

@scohen
Copy link
Contributor

scohen commented Nov 30, 2022

I'd go further for protocol implementations and eliminate them entirely, I've never once needed to alias one. It's noisy and there are a lot of them.

@wodow
Copy link

wodow commented Dec 6, 2022

I desperately want to turn this off -- it's causing a couple of unnoticed bugs per day at the moment.

I will roll back to v0.11 for now but will try to understand how (IDE-independent?) configuration is possible to add a config flag.

@lukaszsamson
Copy link
Collaborator

I decided to completely remove auto aliasing of protocol implementations. This should cut a big part of unwanted items. I'm also experimenting with improvements to displaying of those alias items. labelDetails.description added in LSP 3.17 (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemLabelDetails) seems like a reasonable place to put additional alias metadata. In vscode the result is
Screenshot 2022-12-11 at 00 11 50
WDYT?

@mindok
Copy link

mindok commented Dec 12, 2022

@lukaszsamson - that's a big improvement on both fronts (removing the protocol implementations, and adding the description).

Given the functionality silently updates the code (although in much better ways with the first two changes), I still think it makes sense to have the option to disable it.

@paulanthonywilson
Copy link

I desperately want to turn this off -- it's causing a couple of unnoticed bugs per day at the moment.

I too vote for the ability for this to be disabled, or removed altogether. I am sure that it seemed like a good idea but I don't think I've every found it useful but regularly get caught out by unintentional incorrect aliasing, often in weird places, all the time.

@mvkvc
Copy link

mvkvc commented Mar 30, 2023

Agreed! Currently in a bootcamp and for everyone this is confusing and causes errors. Would really appreciate the option to disable it.

@wodow
Copy link

wodow commented May 3, 2023

I'm now seeing alias nil appear automaticlaly (without a known trigger yet), perhaps since the most recent release (mid April).

@mvkvc
Copy link

mvkvc commented May 4, 2023

Yes alias nil keeps being imported for me as well and very often. In general being able to disable the auto adding of imports would be great for me.

@lukaszsamson
Copy link
Collaborator

PR please. And please post a piece of code that reproduces alias nil

@c4710n
Copy link
Contributor

c4710n commented May 4, 2023

If setting aside the issue with alias nil for now, I'd like contribute the feature for disabling automatic alias insertion.

@LukasKnuth, am I on the right road?

@lukaszsamson
Copy link
Collaborator

elixirLS.autoInsertRequiredAlias looks OK

@wodow
Copy link

wodow commented May 4, 2023 via email

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 a pull request may close this issue.

8 participants