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

API/SPI to apply WorkspaceEdit #7401

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented May 24, 2024

During implementation of new features, for example project dependency modification or some edits, we increasingly sarted to use WorkspaceEdit from the LSP APis, so that the code would apply for both NetBeans IDE and NetBeans running as a NBLS server. Sometimes the action code needs to ensure the changes are applied and possibly saved - but that needs to be perfomed in different ways, depending on whether NB runs as an IDE or as NBLS.

So far, the only possibility was to make a command that returns a WorkspaceEdit to the LSP client (or to a wrapper code in NB IDE) and the client (or wrapper) applies the edit. Sadly ;) there is still no standard implementation how to apply the WorkspaceEdit in the NB code, so the IDE implementation would need to be done manually.

This PR introduces an API to apply the changes + SPI that is implemented for NetBeans IDE (in project.dependencies module) and for LSP server (implemented in nbcode/integrations module). The IDE implementation uses EditorCookie and Document to make textual changes, and Savable to perform the save operation.

The LSP protocol implementation uses standard applyWorkspaceEdit call to perform changes and our custom requestDocumentSave to save resources.

Basic tests for IDE and LSP parts included.

@sdedic sdedic added API Change [ci] enable extra API related tests LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests kind:feature A feature request labels May 24, 2024
@sdedic sdedic added this to the NB23 milestone May 24, 2024
@sdedic sdedic self-assigned this May 24, 2024
@sdedic sdedic requested a review from jhorvath May 27, 2024 06:49
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

Copy link
Contributor

@jhorvath jhorvath left a comment

Choose a reason for hiding this comment

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

Looks good

@sdedic sdedic merged commit 47c2df7 into apache:master May 28, 2024
33 checks passed
@mbien
Copy link
Member

mbien commented May 28, 2024

looks like the language version bump is causing groovy tests to fail since they have to test on JDK 8 due to #4904

I am working on a fix. The recent changes in the build made it easier to bump groovy to more recent JDKs since most pass out of the box now.

@sdedic
Copy link
Member Author

sdedic commented May 28, 2024

@mbien Thanks -- wouldn't it be better if I downgrade for a while lsp.api back to 8 ? Since the language bump was just because of CompletableFuture.failedFuture which I can achieve in a different way.

@mbien
Copy link
Member

mbien commented May 28, 2024

@sdedic we have to bump the remaining few "stuck stuck with 8" tests anyway at some point. I am running them locally now and 98% pass, rest seem to be fixable by updating the golden files (some completion tests are JDK API sensitive for example).

Last time I tried this far more failed due to module system issues, but it seems it is resolved now and it looks much better.

@mbien
Copy link
Member

mbien commented May 28, 2024

this should fix it I think #7415

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change [ci] enable extra API related tests kind:feature A feature request LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants