Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
skovhus merged 21 commits into bash-lsp:master from shabbyrobe:master
May 13, 2022
Merged

Shellcheck integration #342

skovhus merged 21 commits into bash-lsp:master from shabbyrobe:master
May 13, 2022

Conversation

Copy link
Contributor

@shabbyrobe shabbyrobe commented Mar 30, 2022

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.

David-Else, PowerUser64, 616b2f, and s-burris reacted with thumbs up emoji David-Else, PowerUser64, 616b2f, and s-burris reacted with hooray emoji
Copy link
Collaborator

@skovhus skovhus left a 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?

shabbyrobe reacted with thumbs up emoji
Copy link
Contributor Author

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

Copy link

codecov bot commented Apr 5, 2022
edited
Loading

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.

Copy link

y0rune commented Apr 20, 2022

It works! Thank you <3

Copy link

0x0D15 commented Apr 29, 2022

@skovhus Is there anything currently blocking this?

Copy link
Contributor Author

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.

0x0D15, 616b2f, skovhus, and y0rune reacted with thumbs up emoji David-Else reacted with eyes emoji

Copy link
Collaborator

@skovhus skovhus left a 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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

arnaldo2792, David-Else, and s-burris reacted with thumbs up emoji David-Else and PowerUser64 reacted with eyes emoji

Copy link

PowerUser64 commented May 13, 2022
edited
Loading

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!

616b2f reacted with heart emoji

Copy link
Collaborator

@skovhus skovhus left a 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 👏

Copy link
Contributor Author

shabbyrobe commented May 13, 2022
edited
Loading

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

Copy link
Collaborator

skovhus commented May 13, 2022

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 reacted with thumbs up emoji

@skovhus skovhus enabled auto-merge May 13, 2022 12:52
Copy link
Contributor Author

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

Copy link
Contributor Author

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

@skovhus skovhus disabled auto-merge May 13, 2022 12:57
Copy link
Collaborator

skovhus commented May 13, 2022

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

Copy link
Contributor Author

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 skovhus enabled auto-merge May 13, 2022 13:00
Copy link
Collaborator

skovhus commented May 13, 2022

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.

shabbyrobe and others added 18 commits May 13, 2022 23:47
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>
Copy link
Collaborator

skovhus commented May 13, 2022

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

6.1.0 (latest) still support node 12 ;)

Copy link
Collaborator

skovhus commented May 13, 2022

Would execShellScript work that we have in utils?

Copy link
Contributor Author

shabbyrobe commented May 13, 2022
edited
Loading

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.

skovhus reacted with heart emoji

@skovhus skovhus merged commit e55bac8 into bash-lsp:master May 13, 2022
Copy link

y0rune commented May 16, 2022
edited
Loading

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

image

Copy link
Collaborator

skovhus commented May 16, 2022

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

This error seems to be fixed in #394

Copy link

y0rune commented May 16, 2022

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

Copy link

y0rune commented May 24, 2022

Okey I reinstalled it again and it worked. ;)
Screenshot 2022年05月25日 at 00 38 41

616b2f reacted with thumbs up emoji

sebnow added a commit to sebnow/configs that referenced this pull request Sep 2, 2023
- `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 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skovhus skovhus skovhus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /