-
Notifications
You must be signed in to change notification settings - Fork 137
fix: should check type of folders #535
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
Conversation
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 which case is folders not an array? What is it?
I'm asking as the type signature here explicitly says that it should always be an array of LSP.WorkspaceFolder
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.
according to rough looking to lsp specification,
looks it said It can be 'null' if the client supports workspace folders but none are configured.
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.
That case is handled here: https://github.com/bash-lsp/bash-language-server/blob/main/server/src/server.ts#L92
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.
the problem looks was the previous statement before L92.
const folders = this.clientCapabilities.workspace?.workspaceFolders ? await connection.workspace.getWorkspaceFolders() : [] const lintDiagnostics = await this.linter.lint(change.document, folders || [])
this.clientCapabilities.workspace?.workspaceFoldersthis got frominitializeand that istrue.- however
await connection.workspace.getWorkspaceFolders()looks sendworkspace/workspaceFoldersto lsp client, but got{'id': 0, 'jsonrpc': '2.0', 'result': {}} - so that
folderslooks was a{}?! <--------- !!!
so the problem is:
- could it get from
this.clientCapabilities.workspaceFolderswhen/ifthis.clientCapabilities.workspace?.workspaceFoldersis true? - should lsp client response with
result: ........something else or could be{}? - anyway,
{}perhaps that's the direct problem.
and looks others lsp server, i saw they just get workspaces folders from clientCapabilities.workspaceFolders, did not send/get from lsp workspace/workspaceFolders method.
@skovhus that should be a bug (or un-impl feature) of lsp client, i've opened a pr to correct/impl that.
// see yegappan/lsp#114
// but this pr to bash lsp server, perhaps should be better merged too, since varied lsp client may send others type of folders var.
2ed62c5 to
dde3144
Compare
but this pr to bash lsp server, perhaps should be better merged too, since varied lsp client may send others type of folders var.
We shouldn't extend the LSP server here to support faulty clients – we want to follow the LSP standard. But thanks for fixing yegappan/lsp#114
I'm closing this for now, but thanks for looking into it.
closes #516