Skip to content

Change TextEdit to WorkspaceEdit for actionable diagnostics #1957

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.

Sign up for GitHub

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

Jump to bottom
Closed
tgodzik opened this issue Mar 23, 2023 · 3 comments
Closed

Change TextEdit to WorkspaceEdit for actionable diagnostics #1957

tgodzik opened this issue Mar 23, 2023 · 3 comments
Assignees
@ckipp01
Labels
enhancement New feature or request

Comments

@tgodzik
Copy link
Member

tgodzik commented Mar 23, 2023

Is your feature request related to a problem? Please describe.
There is an ongoing work around the compiler to introduce actionable diagnostics and the consensus is that WorkspaceEdit from LSP spec would be much more powerful.

Describe the solution you'd like
Change TextEdit to WorkspaceEdit and also make a fallback mechanism in Metals.
@tgodzik tgodzik added the enhancement New feature or request label Mar 23, 2023
@gabro
Copy link

gabro commented Mar 23, 2023

since we're at it, it would also be good to nest it under a key like workspaceEdit instead of have it at the top level

@ckipp01
Copy link
Contributor

ckipp01 commented Mar 28, 2023

Just a heads up so I don't step on anyone's toes. I'm working on this right now.

ckipp01 added a commit to ckipp01/scala-cli that referenced this issue Mar 28, 2023
@ckipp01
refactor: change data in bsp.Diagnostic to contain a workspaceEdit
6de8222 
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 VirtusLab#1957
@ckipp01 ckipp01 mentioned this issue Mar 28, 2023
Closed
ckipp01 added a commit to ckipp01/scala-cli that referenced this issue Apr 11, 2023
@ckipp01
refactor: change data in bsp.Diagnostic to contain a workspaceEdit
66018c7 
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
ckipp01 added a commit to ckipp01/scala-cli that referenced this issue Apr 12, 2023
@ckipp01
refactor: change data in bsp.Diagnostic to contain a workspaceEdit
b60ef76 
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
@tgodzik
Copy link
Member Author

tgodzik commented Jul 24, 2023

This is now fixed! Thanks @ckipp01

@tgodzik tgodzik closed this as completed Jul 24, 2023
@tgodzik tgodzik moved this to Done in Scala CLI Board Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ckipp01 ckipp01

Labels
enhancement New feature or request
Projects
Scala CLI Board
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: change data in bsp.Diagnostic to contain a workspaceEdit ckipp01/scala-cli
3 participants
@gabro @tgodzik @ckipp01