Skip to content
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

Fix auto detection for path of shellcheck.exe on Windows #563

Merged
merged 2 commits into from
Nov 27, 2022
Merged

Fix auto detection for path of shellcheck.exe on Windows #563

merged 2 commits into from
Nov 27, 2022

Conversation

jfcherng
Copy link
Contributor

@jfcherng jfcherng commented Nov 26, 2022

Issues

  • There is no bash/which on Windows, by default.
  • Even if using the bash/which from git-bash or WSL, the returned path is Unix-style. And Node.js on Windows is not happy with it.

Solutions

  • Use cmd.exe as the default shell on Windows.
  • Use the which package to find shellcheck in a cross-platform way.

@codecov
Copy link

codecov bot commented Nov 26, 2022

Codecov Report

Merging #563 (d411668) into main (f1352fc) will decrease coverage by 0.18%.
The diff coverage is 55.55%.

❗ Current head d411668 differs from pull request most recent head ad1e7d0. Consider uploading reports for the commit ad1e7d0 to get more accurate results

@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
- Coverage   74.85%   74.66%   -0.19%     
==========================================
  Files          19       19              
  Lines         676      679       +3     
  Branches      119      121       +2     
==========================================
+ Hits          506      507       +1     
+ Misses        154      153       -1     
- Partials       16       19       +3     
Impacted Files Coverage Δ
server/src/linter.ts 89.04% <50.00%> (+2.96%) ⬆️
server/src/util/sh.ts 84.21% <57.14%> (-4.26%) ⬇️
server/src/analyser.ts 84.00% <0.00%> (-0.98%) ⬇️
server/src/server.ts 58.76% <0.00%> (-0.62%) ⬇️
server/src/util/platform.ts 100.00% <0.00%> (+100.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@skovhus skovhus enabled auto-merge November 27, 2022 00:00
@skovhus skovhus self-requested a review November 27, 2022 00:00
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.

Thank you so much for fixing this so fast! This was on my todo list. 👏

@skovhus skovhus merged commit 2c2503c into bash-lsp:main Nov 27, 2022
@skovhus
Copy link
Collaborator

skovhus commented Nov 27, 2022

@jfcherng any specific reason for using version 2 of the which library? I see that there is a version 3, but haven't looked if there are any breaking changes (e.g. like only supporting ESM)

@jfcherng
Copy link
Contributor Author

jfcherng commented Nov 27, 2022

@jfcherng any specific reason for using version 2 of the which library? I see that there is a version 3, but haven't looked if there are any breaking changes (e.g. like only supporting ESM)

I tried v3 at first. It works fine without any code change but it requires Node v14, so it will failed the test for Node v12 here.

@jfcherng jfcherng deleted the fix/finding-shellcheck-windows branch November 27, 2022 04:14
@skovhus skovhus mentioned this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants