Issue #132: Proposal for workspaceFolders#133
Issue #132: Proposal for workspaceFolders#133spoenemann merged 1 commit intoeclipse-lsp4j:masterfrom
Conversation
mickaelistria
force-pushed
the
132-workspaceFolders
branch
from
650c353 to
70fa8f9
Compare
spoenemann
left a comment
spoenemann
left a comment
There was a problem hiding this comment.
Thanks for the PR!
In addition to the remarks below, I would propose to add comments to the new classes and properties making clear that this API is in a "proposed" state and might be changed in the future. We could also use the com.google.common.annotations.Beta annotation for that.
| /** | ||
| * Capabilities specific to the `workspace/didChangeWorkspaceFolders` notification. | ||
| */ | ||
| WorkspaceFoldersOptions workspaceFolders |
There was a problem hiding this comment.
In the current version of the proposed extension, the workspaceFolders options are wrapped in an additional workspace property. This should be a new class named WorkspaceServerCapabilities.
There was a problem hiding this comment.
I think that's more something to fix on the proposed extension side ;)
| /** | ||
| * The server has support for workspace folders | ||
| */ | ||
| Boolean workspaceFolders |
There was a problem hiding this comment.
This should be named supported.
| /** | ||
| * The associated URI for this workspace folder. | ||
| */ | ||
| String uri |
There was a problem hiding this comment.
This should be @NonNull.
| * the workspace folders otherwise. | ||
| */ | ||
| @JsonRequest("workspace/workspaceFolders") | ||
| CompletableFuture<List<WorkspaceFolder>> workspaceFolders(); |
There was a problem hiding this comment.
Adding a method here breaks existing implementations. Since the protocol extension is still in a "proposed" state, we should avoid breaking changes. Either move the method to a separate interface or add a default implementation here. I prefer the second option.
| * registered to receive this notification it first. | ||
| */ | ||
| @JsonNotification | ||
| void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params); |
There was a problem hiding this comment.
This breaks existing implementations, see above.
mickaelistria
force-pushed
the
132-workspaceFolders
branch
from
70fa8f9 to
6105cf4
Compare
|
PR updated. Only the first comment has not been fixed since I'm waiting for a clarification on the proposal: microsoft/language-server-protocol#298 (comment) |
spoenemann
left a comment
spoenemann
left a comment
There was a problem hiding this comment.
Looks good, thanks!
Let's wait for a reply by @dbaeumer regarding ServerCapabilities before we merge this.
|
Commented in microsoft/language-server-protocol#298 (comment) |
Signed-off-by: Mickael Istria <[email protected]>
mickaelistria
force-pushed
the
132-workspaceFolders
branch
from
6105cf4 to
4ae0ab3
Compare
|
Thanks for the clarification @dbaeumer . |
|
👍 |
3 participants
Signed-off-by: Mickael Istria [email protected]