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

Completions should prepend, not replace as it is for Scala 2 #18803

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Oct 31, 2023

By default, in Scala 2 completion prepend and only replace if there is exact match. This PR unifies the experience between the Scala versions. It seems more intuitive to work that way.

The change will be as follows (the order is: new logic, old logic and scala 2 logic):

Screen.Recording.2023-10-31.at.15.30.31.mov

In the future, it should be improved by changing implementation to use InsertReplaceEdit instead of TextEdit. This will allow users to decide which behaviour they prefer, but this has to be first implemented in metals to properly handle the new logic, but for now the key is to unify behaviours. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#insertReplaceEdit

@rochala rochala added the area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools label Oct 31, 2023
Copy link
Contributor

@kasiaMarek kasiaMarek left a comment

Choose a reason for hiding this comment

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

I assume by exactly match you mean:

print@@ln

Could you add a test for the case were you strip the suffix (for the sake of documentation at least)?

Comment on lines 173 to 175
val textEdit = range
.map(lspRange => new TextEdit(lspRange, newText))
.getOrElse(new TextEdit(editRange, newText))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
val textEdit = range
.map(lspRange => new TextEdit(lspRange, newText))
.getOrElse(new TextEdit(editRange, newText))
val textEdit = new TextEdit(range.getOrElse(editRange), newText)

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik tgodzik added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 2, 2023
@rochala rochala merged commit dc1e35a into scala:main Nov 2, 2023
18 checks passed
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur pushed a commit that referenced this pull request Jun 23, 2024
By default, in Scala 2 completion prepend and only replace if there is
exact match. This PR unifies the experience between the Scala versions.
It seems more intuitive to work that way.

The change will be as follows (the order is: new logic, old logic and
scala 2 logic):


https://github.com/lampepfl/dotty/assets/48657087/c037c322-5613-4b95-a6e5-090b4e8827b6


In the future, it should be improved by changing implementation to use
`InsertReplaceEdit` instead of `TextEdit`. This will allow users to
decide which behaviour they prefer, but this has to be first implemented
in metals to properly handle the new logic, but for now the key is to
unify behaviours.
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#insertReplaceEdit
[Cherry-picked dc1e35a]
WojciechMazur added a commit that referenced this pull request Jun 23, 2024
…2" to LTS (#20756)

Backports #18803 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants