-
Notifications
You must be signed in to change notification settings - Fork 337
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
improvement: Make including detail in completion label configurable #6986
Conversation
Seems like the changes to EDIT: Some investigation leads me to believe that Metals is falling back to the Scala presentation compiler instead of mtags. Unsure if there's anything that can be done about that until mtags is updated for 3.3.4. |
From 3.3.4 onwards we use mtags published by the compiler itself to avoid having to publish it to every version of the compiler. |
Ach, and we usually just port them ourselves, so no need to worry about it |
mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompilerConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from the MiMa failure 🎉
80387b5
to
fad1b04
Compare
Addressed the default - are we sure this change doesn't require any tests? I'd add them, I'm just not sure where they belong. |
fad1b04
to
041ab36
Compare
You should be able to add the tests to the cross module, you would need to change the config like in https://github.com/scalameta/metals/blob/main/tests/cross/src/test/scala/tests/pc/CompletionSnippetNegSuite.scala#L10 |
Some LSP clients (such as Emacs using Eglot and Corfu) use the label field to insert a completion if completionItem/resolve has not returned by the time the completion is selected. Including the contents of the detail property in the label causes it to be included in the file itself in these clients. To remedy this, we define a isDetailIncludedInLabel option under compilerOptions to make this behaviour toggleable. We also disable this behaviour by default in Emacs, since this is where the undesirable behaviour was located. Fixes scalameta#6849.
041ab36
to
f0ccc7b
Compare
That should do for test cases - let me know if there's anything else you think I should check. |
I added one test, fixed the added one on Scala 3 and removed the detail also from scope completions. Thanks for working on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I will merge tomorrow once it passes (unless I broke something)
Some LSP clients (such as Emacs using Eglot and Corfu) use the label field to insert a completion if
completionItem/resolve
has not returned by the time the completion is selected. Including the contents of the detail property in the label causes it to be included in the file itself in these clients. To remedy this, we define aisDetailIncludedInLabel
option undercompilerOptions
to make this behaviour toggleable. We also disable this behaviour by default in Emacs, since this is where the undesirable behaviour was located.TODO:
Fixes #6849.