refactor: change data in bsp.Diagnostic to contain a workspaceEdit #1969

ckipp01 · 2023-03-28T07:01:14Z

This change is part of the discussion that we had at the Scala Tooling
Summit where we decided that using a WorkspaceEdit rather than a
TextEdit provided a more future-proof way of actionable diagnostics
since it's richer. This PR makes that changes, but in a limited way.

It only actually changes this when we go from the Diagnostic to a
BSP Diagnostic. It doesn't for example change the TextEdit that the
internal representation of Diagnostic has. That's left as is to
minimize changes internally.
Right now really only changes are supported in the WorkspaceEdit
mainly because that's all we really need at this time. In the future
this could be expanded to use the richer documentChanges if the BSP
client supports it.
This also changes the structure of data slightly to not have the
actual edit be at the top level and instead nests it under an edit
key. There is no gaurantee that we won't have other keys in data in
the future, so this sort of allows for that and ensures that we don't
get stuck with only being able to have the actual WorkspaceEdit at
the top level in the data object.

Fixes #1957

modules/build/src/main/scala/scala/build/bsp/protocol/DiagnosticData.scala

modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala

modules/build/src/main/scala/scala/build/bsp/protocol/DiagnosticData.scala

ckipp01

Still need to fix the tests, but I wanted to get the full structure up here and get some feedback.

modules/build/src/main/scala/scala/build/bsp/protocol/WorkspaceEdit.scala

modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala

modules/build/src/main/scala/scala/build/bsp/protocol/ResourceOperation.scala

This change is part of the discussion that we had at the Scala Tooling Summit where we decided that using a `WorkspaceEdit` rather than a `TextEdit` provided a more future-proof way of actionable diagnostics since it's richer. This PR makes that changes, but in a limited way. - It only actually changes this when we go from the `Diagnostic` to a BSP Diagnostic. It doesn't for example change the `TextEdit` that the internal representation of Diagnostic has. That's left as is to minimize changes internally. - Right now really only `changes` are supported in the `WorkspaceEdit` mainly because that's all we really need at this time. In the future this could be expanded to use the richer `documentChanges` if the BSP client supports it. - This also changes the structure of `data` slightly to not have the actual edit be at the top level and instead nests it under an `edit` key. There is no gaurauntee that we won't have other keys in `data` in the future, so this sort of allows for that and ensures that we don't get stuck with only being able to have the actual `WorkspaceEdit` at the top level in the `data` object. Fixes VirtusLab#1957

lwronski · 2023-04-13T10:41:22Z

I saw there is issue with generating the native image and therefore native tests fail, I think that reflection configuration is missing for graalvm. Let me check and I will get back to you

lwronski · 2023-04-13T11:40:38Z

I saw there is issue with generating the native image and therefore native tests fail, I think that reflection configuration is missing for graalvm. Let me check and I will get back to you

I think my commit should resolve the issues with the native tests. It was added the lsp4j dependency and then used Gson to serialize a class which uses reflection. However, for the GraalVM image, the class/method that uses reflection must be specified in reflect-config.json

ckipp01 · 2023-04-14T06:10:54Z

I think my commit should resolve the issues with the native tests. It was added the lsp4j dependency and then used Gson to serialize a class which uses reflection. However, for the GraalVM image, the class/method that uses reflection must be specified in reflect-config.json

Ahh perfect, thanks! This should be ready to review now but I'll also work on a draft pr in Metals just to show that this still works as expected there as well.

lwronski · 2023-04-14T08:03:30Z

I think my commit should resolve the issues with the native tests. It was added the lsp4j dependency and then used Gson to serialize a class which uses reflection. However, for the GraalVM image, the class/method that uses reflection must be specified in reflect-config.json

Ahh perfect, thanks! This should be ready to review now but I'll also work on a draft pr in Metals just to show that this still works as expected there as well.

Thanks for your contribution @ckipp01 , overall LGTM. So I wait until you finish your work in Metals.

Previously in scala-cli the diagnostic `data` for actionable diagnostics were `textEdit`s, whereas now they are nested under `edits` and are `workspaceEdit`s. This change will ensure they still work for the old format and also for the new. This relates to the changes in VirtusLab/scala-cli#1969.

ckipp01 · 2023-04-24T06:30:00Z

Alright, so this should be ready to go now. I have a PR in Metals that shows that it can still handle the old way of doing it, and we can add another test in there as well for the new way of doing it when we get a new release.

Gedochao

LGTM
edit: will merge this after scalameta/metals#5173 is ready

ckipp01 · 2023-04-25T07:28:32Z

will merge this after scalameta/metals#5173 is ready

That pr is actually ready. The only reason it's marked as a draft is because it's waiting for this one 😆 .

tgodzik · 2023-04-25T09:06:09Z

That pr is actually ready. The only reason it's marked as a draft is because it's waiting for this one laughing .

och, but if we don't merge it in Metals it will be broken there 😅 We should merge the Metals one first IMHO

Previously in scala-cli the diagnostic `data` for actionable diagnostics were `textEdit`s, whereas now they are nested under `edits` and are `workspaceEdit`s. This change will ensure they still work for the old format and also for the new. This relates to the changes in VirtusLab/scala-cli#1969.

tgodzik · 2023-07-05T09:48:38Z

The metals PR is merged 🎉 Do you need help finishing this one @ckipp01 ?

ckipp01 · 2023-07-05T09:54:17Z

The metals PR is merged 🎉 Do you need help finishing this one @ckipp01 ?

Hey! I'll actually jump back on this now. I'll get to it this week.

This is a simplified version of VirtusLab#1969 that gets rid of the `ActionableDiagnostic` that is used internally and instead just ensures that any time a `TextEdit` is created there is a `title`. Then we take all the information already contained in the `Diagnostic` and make a `ScalaAction` from it to be forwarded via the diagnostic data.

ckipp01 · 2023-07-12T12:00:07Z

Superseded by #2284

This is a simplified version of VirtusLab#1969 that gets rid of the `ActionableDiagnostic` that is used internally and instead just ensures that any time a `TextEdit` is created there is a `title`. Then we take all the information already contained in the `Diagnostic` and make a `ScalaAction` from it to be forwarded via the diagnostic data.

* feat: use the new ScalaAction from BSP4J This is a simplified version of #1969 that gets rid of the `ActionableDiagnostic` that is used internally and instead just ensures that any time a `TextEdit` is created there is a `title`. Then we take all the information already contained in the `Diagnostic` and make a `ScalaAction` from it to be forwarded via the diagnostic data. * fix: make sure reflect config is updated * refactor: remove unused imports * fix: make sure data kind is set

ckipp01 commented Mar 28, 2023

View reviewed changes

modules/build/src/main/scala/scala/build/bsp/protocol/DiagnosticData.scala Outdated Show resolved Hide resolved

modules/cli/src/main/scala/scala/cli/internal/CliLogger.scala Outdated Show resolved Hide resolved

Gedochao reviewed Mar 28, 2023

View reviewed changes

modules/build/src/main/scala/scala/build/bsp/protocol/DiagnosticData.scala Outdated Show resolved Hide resolved

modules/build/src/main/scala/scala/build/bsp/protocol/DiagnosticData.scala Outdated Show resolved Hide resolved

ckipp01 force-pushed the workspaceEdit branch from 6de8222 to 66018c7 Compare April 11, 2023 11:06

ckipp01 commented Apr 12, 2023

View reviewed changes

ckipp01 requested a review from Gedochao April 12, 2023 08:57

ckipp01 added 3 commits April 12, 2023 21:53

refactor: add in the various ResourceOperations

20c4e68

refactor: just use lsp4j

cea5c71

ckipp01 force-pushed the workspaceEdit branch from ec0261b to cea5c71 Compare April 12, 2023 19:56

Add reflect configuration after adding lsp4j dependency

7bb428c

ckipp01 mentioned this pull request Apr 24, 2023

refactor: account for new diagnosticData shape scalameta/metals#5173

Closed

ckipp01 marked this pull request as ready for review April 24, 2023 06:29

ckipp01 requested a review from lwronski April 24, 2023 06:30

ckipp01 mentioned this pull request Apr 24, 2023

[Progress] - start utilizing data in diagnostics to forward "quickfixes" to tools scala/scala3#17337

Closed

5 tasks

Gedochao approved these changes Apr 25, 2023

View reviewed changes

ckipp01 marked this pull request as draft July 6, 2023 13:58

ckipp01 mentioned this pull request Jul 12, 2023

feat: use the new ScalaAction from BSP4J #2284

Merged

ckipp01 closed this Jul 12, 2023

ckipp01 deleted the workspaceEdit branch July 17, 2023 14:52

refactor: change data in bsp.Diagnostic to contain a workspaceEdit #1969

refactor: change data in bsp.Diagnostic to contain a workspaceEdit #1969

Uh oh!

Conversation

ckipp01 commented Mar 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ckipp01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lwronski commented Apr 13, 2023

Uh oh!

lwronski commented Apr 13, 2023

Uh oh!

ckipp01 commented Apr 14, 2023

Uh oh!

lwronski commented Apr 14, 2023

Uh oh!

ckipp01 commented Apr 24, 2023

Uh oh!

Gedochao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckipp01 commented Apr 25, 2023

Uh oh!

tgodzik commented Apr 25, 2023

Uh oh!

tgodzik commented Jul 5, 2023

Uh oh!

ckipp01 commented Jul 5, 2023

Uh oh!

ckipp01 commented Jul 12, 2023

Uh oh!

Uh oh!

Gedochao left a comment •

edited

Loading