-
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
@@ -63,8 +63,10 @@ export default class Linter { | |||
folders: LSP.WorkspaceFolder[], | |||
): Promise<ShellcheckResult> { | |||
const args = ['--format=json1', '--external-sources', `--source-path=${this.cwd}`] | |||
for (const folder of folders) { | |||
args.push(`--source-path=${folder.name}`) | |||
if (Array.isArray(folders)) { |
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 || [])
so the problem is:
|
and looks others lsp server, i saw they just get workspaces folders from |
@skovhus that should be a bug (or un-impl feature) of lsp client, i've opened a pr to correct/impl that. // but this pr to bash lsp server, perhaps should be better merged too, since varied lsp client may send others type of |
2ed62c5
to
dde3144
Compare
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