-
Notifications
You must be signed in to change notification settings - Fork 137
Shellcheck integration #342
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.
Thanks for adding this! Can I get you to run yarn lint
to auto fix some linting issues?
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 Report
@@ 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
Continue to review full report at Codecov.
|
y0rune
commented
Apr 20, 2022
It works! Thank you <3
0x0D15
commented
Apr 29, 2022
@skovhus Is there anything currently blocking this?
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.
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.
It would be great to add a few unit tests. Thanks again for adding support for this.
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.
I've fixed the test error with Node 14. Can someone please approve the workflow?
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!
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.
I just tried this out and it works really nicely! Thanks for adding this 👏
My pleasure! Thanks for reviewing and thank you for the project! Would you like me to rebase the PR onto master now?
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?
Yikes, it looks like github borked the rebase and replaced it with a merge commit. I will fix and force push.
Ah whups, auto merge enabled; should I just let it go and not muck around with it?
Let me know when you are ready for this to be merged. :) Again, we are not super strict on rebasing/merging.
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 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 <skovhus@users.noreply.github.com>
Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com>
Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com>
Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com>
Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com>
Unfortunately, they just dropped support for Node 12 just 3 days ago 😆
6.1.0 (latest) still support node 12 ;)
Would execShellScript
work that we have in utils?
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.
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 bash-ide-vscode@1.13.0: 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
Did you run a clean yarn install
? Try removing node_modules.
This error seems to be fixed in #394
y0rune
commented
May 16, 2022
Yup, I removed the all your repo and reclone it ;)
y0rune
commented
May 24, 2022
Okey I reinstalled it again and it worked. ;)
Screenshot 2022年05月25日 at 00 38 41
- `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
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.