-
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
Scala cli actionable diagnostic #4297
Scala cli actionable diagnostic #4297
Conversation
metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
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.
Thanks for the great work! I left a bunch of comments, but mostly minor things.
metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/scalacli/ScalaCli.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/scalacli/ScalaCli.scala
Outdated
Show resolved
Hide resolved
@@ -54,19 +56,31 @@ abstract class BaseCodeActionLspSuite( | |||
changeFile: String => String = identity, | |||
expectError: Boolean = false, | |||
filterAction: CodeAction => Boolean = _ => true, | |||
scalaCli: Boolean = false, |
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.
Could you maybe add layout as a parameter? Boolean scalaCLi
is not clear on what it does.
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.
In BaseCodeActionLspSuite
layout is created base on few parameters such as: scalacOptionsJson
, scalafixConf
, path
, input
. I'm not sure what you expect, do you want that users how to pass explicitly layout: String
?
For now, I changed name from scalaCli
to scalaCliLayout
.
tests/unit/src/main/scala/tests/codeactions/BaseCodeActionLspSuite.scala
Outdated
Show resolved
Hide resolved
@@ -54,19 +56,31 @@ abstract class BaseCodeActionLspSuite( | |||
changeFile: String => String = identity, | |||
expectError: Boolean = false, | |||
filterAction: CodeAction => Boolean = _ => true, | |||
scalaCli: Boolean = false, | |||
skipDiff: Boolean = false, |
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.
skipDiff: Boolean = false, | |
assertion: Option[(Boolean, String)], |
Maybe something like this? Have separate assertion to make sure skipDiff
is not used by mistake?
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.
I don't understand why you used Boolean there, so I changed the assertion type a bit to:
assertion: Option[Unit => Unit]
which allows to override default assert in test
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.
I was thinking:
assertion.map{ case (condition, msg) => assert(condition, msg)}
This way we would steer with the types what needs to be done.
metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala
Outdated
Show resolved
Hide resolved
199fed2
to
df9a7cb
Compare
metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ScalacDiagnostic.scala
Outdated
Show resolved
Hide resolved
cb8bd0f
to
d6c2ee7
Compare
|
||
check( | ||
"actionable-diagnostic-update", | ||
s"""|//> <<>>using lib "com.lihaoyi::os-lib:${oldOsLibVersion.repr}" |
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.
What's the actual diagnostic tied to this look like?
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.
[Trace - 02:46:40 PM] Received notification 'build/publishDiagnostics'
Params: {
"textDocument": {
"uri": "file:///private/tmp/scala-demo/Hello.scala"
},
"buildTarget": {
"uri": "file:/private/tmp/scala-demo/.scala-build/?id\u003dproject_bd2c96d2de"
},
"diagnostics": [
{
"range": {
"start": {
"line": 0,
"character": 15
},
"end": {
"line": 0,
"character": 40
}
},
"severity": 4,
"source": "scala-cli",
"message": "com.lihaoyi::os-lib:0.7.8 is outdated, update to 0.8.1\n com.lihaoyi::os-lib:0.7.8 -\u003e com.lihaoyi::os-lib:0.8.1",
"data": {
"range": {
"start": {
"line": 0,
"character": 15
},
"end": {
"line": 0,
"character": 40
}
},
"newText": "com.lihaoyi::os-lib:0.8.1"
}
}
],
"reset": false
}
} | ||
|
||
object ActionableDiagnostic { | ||
def title = "Apply suggestion" |
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.
I commented on it in the tests, but the diagnostic message mention what the applied suggestion will be?
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.
For now no, so I updated it and use AnnotatedTextEdit to send some more additional message about the applied suggestion. I will update this PR after release scala-cli
to 0.1.17
.
For dependency actionable diagnostic - in field AnnotatedTextEdit. annotationId
I set value to: update dependency to 0.8.1
.
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.
Annotated text edit seems a bit more complex and might not be supported by all the clients, what about doing:
Fix: <diagnostic message>
truncated to a sensible length?
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.
} | ||
|
||
object ActionableDiagnostic { | ||
def title = "Apply suggestion" |
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.
Annotated text edit seems a bit more complex and might not be supported by all the clients, what about doing:
Fix: <diagnostic message>
truncated to a sensible length?
tests/unit/src/test/scala/tests/codeactions/ActionableDiagnosticsSuite.scala
Outdated
Show resolved
Hide resolved
7d2fdac
to
ba4118d
Compare
ba4118d
to
b1686e2
Compare
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!
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.
Great work @lwronski. Excited to see more things like this.
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/codeactions/ActionableDiagnostic.scala
Outdated
Show resolved
Hide resolved
0467db0
to
0791d85
Compare
0791d85
to
a2c1300
Compare
a2c1300
to
faa07fe
Compare
This PR adds support for actionable diagnostic from
scala-cli
. These are diagnostics that contain information that allows users to fix some configs/directives byQuick Fix
actions in metals.For now, in
scala-cli
there is only implemented actionable diagnostics for updating lib in using directives, and it works in this way with metals:Screen.Recording.2022-08-24.at.19.57.06.mov
I added
didSave
flag tocheck
method, because when actionable diagnostics were applied in unit tests and saved content to the file, thescala-cli
reload bsp server and at the same time metals expect to getCompile Result
from server but fail, because bsp server is down. I attach error logs:To fix it, I just skip saving updating files to disk, and in this way, I avoiding to reload of bsp. I don't find any better solution to fix it in my test
ActionableDiagnosticsSuite
.