Shellcheck integration #342

shabbyrobe · 2022-03-30T10:03:32Z

Following on from discussion in #272 and #104, here is a PR that offers some preliminary integration.

I've updated to merge diagnostics from treesitter and shellcheck as requested (naively by concatenating at first, can get fancier if desired). I still want to add some tests. Happy to make any other changes needed.

skovhus

Thanks for adding this! Can I get you to run yarn lint to auto fix some linting issues?

shabbyrobe · 2022-04-05T09:35:24Z

Ah of course, my apologies. I have now linted and rebased with master. Please let me know if there's anything else you'd like to see

codecov · 2022-04-05T16:39:42Z

Codecov Report

Merging #342 (0f607ba) into master (763d8e0) will increase coverage by 1.04%.
The diff coverage is 81.17%.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   72.91%   73.96%   +1.04%     
==========================================
  Files          18       19       +1     
  Lines         576      653      +77     
  Branches       94      112      +18     
==========================================
+ Hits          420      483      +63     
- Misses        135      144       +9     
- Partials       21       26       +5

Impacted Files	Coverage Δ
vscode-client/src/extension.ts	`0.00% <0.00%> (ø)`
server/src/server.ts	`57.44% <41.66%> (-1.80%)`	⬇️
server/src/linter.ts	`88.40% <88.40%> (ø)`
server/src/config.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6c7c74...0f607ba. Read the comment docs.

y0rune · 2022-04-20T18:29:10Z

It works! Thank you <3

0x0D15 · 2022-04-29T05:00:15Z

@skovhus Is there anything currently blocking this?

shabbyrobe · 2022-04-29T05:02:14Z

My feeling was that it probably merited some tests. I just haven't had time to come back to it yet and fill the gap, hence why I haven't pushed harder to get it merged.

skovhus

It would be great to add a few unit tests. Thanks again for adding support for this.

shabbyrobe · 2022-04-30T13:23:32Z

I've added some tests; looks like the workflow needs to be approved again? Please let me know if there are any other tests you'd like to see added, or any changes you want me to make.

shabbyrobe · 2022-05-01T01:50:40Z

I've fixed the test error with Node 14. Can someone please approve the workflow?

PowerUser64 · 2022-05-13T00:37:38Z

Just want to make sure this doesn't get forgotten about. A lot of people are looking forward to seeing this in release.
Thanks for all your work @shabbyrobe and @skovhus!

.github/workflows/verify.yml

server/src/linter.ts

server/src/__tests__/config.test.ts

server/src/linter.ts

skovhus

I just tried this out and it works really nicely! Thanks for adding this 👏

shabbyrobe · 2022-05-13T12:38:23Z

My pleasure! Thanks for reviewing and thank you for the project! Would you like me to rebase the PR onto master now?

server/src/__tests__/linter.test.ts

skovhus · 2022-05-13T12:46:28Z

My pleasure! Thanks for reviewing and thank you for the project! Would you like me to rebase the PR onto master now?

We are not to strict on rebasing, but if you want that would be great. I added one small suggestion.

There also seems to be some formatting issue, can you run yarn lint locally?

shabbyrobe · 2022-05-13T12:53:26Z

Yikes, it looks like github borked the rebase and replaced it with a merge commit. I will fix and force push.

shabbyrobe · 2022-05-13T12:53:57Z

Ah whups, auto merge enabled; should I just let it go and not muck around with it?

skovhus · 2022-05-13T12:58:01Z

Let me know when you are ready for this to be merged. :) Again, we are not super strict on rebasing/merging.

shabbyrobe · 2022-05-13T12:59:43Z

I guess it's just a question of whether you want me to get rid of that merge commit or not. If it doesn't bother you, I'm happy to leave it. If it does, I'll do the git rebase dance 😄

skovhus · 2022-05-13T13:00:56Z

I guess it's just a question of whether you want me to get rid of that merge commit or not. If it doesn't bother you, I'm happy to leave it. If it does, I'll do the git rebase dance 😄

I've gotten more relaxed over the years :D

If you have time then please have a look at #388, I would like your thoughts on the README update.

This was causing an error on Node 14, it was incorrect but appeared to work on later versions

Co-authored-by: Kenneth Skovhus <[email protected]>

skovhus · 2022-05-13T13:49:14Z

Unfortunately, they just dropped support for Node 12 just 3 days ago 😆

6.1.0 (latest) still support node 12 ;)

skovhus · 2022-05-13T13:50:10Z

Would execShellScript work that we have in utils?

shabbyrobe · 2022-05-13T13:52:45Z

I don't think so, not without pretty much the same issue, as it would need to be extended for stdin support. executeShellScript seems to have a subtly different purpose to this one, which runs shellcheck taking a full bash script as standard input.

I think I've fixed the test issue now though, the tests pass on node 12, 14, 16 and 18 on my local machine.

bash-lsp/bash-language-server#342

y0rune · 2022-05-16T09:29:11Z

After checking the latest commit (0f607ba) I got the error :(

yorune@Gentoo-d9yc132 ~/git/bash-language-server master $ sudo yarn install; sudo yarn run reinstall-server
yarn install v1.22.10
warning ../../package.json: No license field
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to
avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
[1/4] Resolving packages...
success Already up-to-date.
$ cd server && yarn install && cd ../vscode-client && yarn install && cd ..
yarn install v1.22.10
warning ../../../package.json: No license field
[1/5] Validating package.json...
[2/5] Resolving packages...
success Already up-to-date.
Done in 0.08s.
yarn install v1.22.10
warning ../../../package.json: No license field
[1/5] Validating package.json...
warning [email protected]: The engine "vscode" appears to be invalid.
[2/5] Resolving packages...
success Already up-to-date.
Done in 0.09s.
Done in 0.64s.
yarn run v1.22.10
warning ../../package.json: No license field
$ npm uninstall -g bash-language-server && yarn compile && npm i -g ./server

removed 1 package, and audited 1 package in 135ms

found 0 vulnerabilities
warning ../../package.json: No license field
$ tsc -b

added 1 package, and audited 3 packages in 359ms

found 0 vulnerabilities
Done in 1.22s.

yorune@Gentoo-d9yc132 ~/git/bash-language-server master $ bash-language-server -v
Version is 2.1.0
(node:4246) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/deps/undici/undici:4816
            throw new TypeError("Failed to parse URL from " + input, { cause: err });
                  ^

TypeError: Failed to parse URL from /home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.wasm
    at new Request (node:internal/deps/undici/undici:4816:19)
    at Agent2.fetch2 (node:internal/deps/undici/undici:5544:29)
    ... 4 lines matching cause stack trace ...
    at /home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.js:1:144
    at Object.<anonymous> (/home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.js:1:170)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10) {
  [cause]: TypeError [ERR_INVALID_URL]: Invalid URL
      at new NodeError (node:internal/errors:377:5)
      at URL.onParseError (node:internal/url:563:9)
      at new URL (node:internal/url:643:5)
      at new Request (node:internal/deps/undici/undici:4814:25)
      at Agent2.fetch2 (node:internal/deps/undici/undici:5544:29)
      at Object.fetch (node:internal/deps/undici/undici:6372:20)
      at fetch (node:internal/bootstrap/pre_execution:199:25)
      at /home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.js:1:15041
      at /home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.js:1:15262
      at /home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.js:1:144 {
    input: '/home/yorune/repo/bash-language-server/server/node_modules/web-tree-sitter/tree-sitter.wasm',
    code: 'ERR_INVALID_URL'
  }
}

Node.js v18.1.0

skovhus · 2022-05-16T09:48:24Z

Did you run a clean yarn install? Try removing node_modules.

This error seems to be fixed in #394

y0rune · 2022-05-16T11:02:35Z

Yup, I removed the all your repo and reclone it ;)

y0rune · 2022-05-24T22:39:24Z

Okey I reinstalled it again and it worked. ;)

- `null-ls` no longer maintained[^1] - `bash-lsp` supports shellcheck for diagnostics[^2] [^1]: jose-elias-alvarez/null-ls.nvim#1621 [^2]: bash-lsp/bash-language-server#342

David-Else mentioned this pull request Apr 2, 2022

Add Rename Symbol Capability #161

Closed

skovhus reviewed Apr 5, 2022

View reviewed changes

skovhus reviewed Apr 29, 2022

View reviewed changes