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: do not overwrite user-provided shellcheck --shell (#1064) #1133

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

chrisgrieser
Copy link
Contributor

This is an attempt to fix #1064. I am really not familiar with the inner working of LSPs, but the fix appears to be simple enough, if I am not mistaken here.

@Kyren223
Copy link

Any updates on this, I have the same issue (the LSP doesn't load on zsh shebangs)

@skovhus
Copy link
Collaborator

skovhus commented Jan 2, 2025

This PR should be an okay workaround, but we should already infer the shell name based on the file name and the shebang. Can you come with an example file where this doesn't work as intended?

@chrisgrieser
Copy link
Contributor Author

chrisgrieser commented Jan 2, 2025

we should already infer the shell name based on the file name and the shebang

That's exactly the problem. If the file is inferred to be a zsh file, --shell=zsh gets passed which stops shellcheck from working.

To force shellcheck to work with zsh files, you need to explicitly pass --shell=bash, which the bash ls currently does not allow. Thus this PR.

@skovhus
Copy link
Collaborator

skovhus commented Jan 2, 2025

Thanks, that makes sense. I see that we actually fall back to using bash if we cannot infer the dialect. We could consider doing the same for zsh files, although your approach is safer.

@skovhus skovhus self-requested a review January 2, 2025 23:15
@skovhus skovhus enabled auto-merge January 2, 2025 23:16
@skovhus
Copy link
Collaborator

skovhus commented Jan 2, 2025

Sorry for the delay here...

@skovhus skovhus merged commit ada88bb into bash-lsp:main Jan 2, 2025
4 checks passed
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.13%. Comparing base (82b964f) to head (5164c63).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1133      +/-   ##
==========================================
+ Coverage   81.09%   81.13%   +0.03%     
==========================================
  Files          29       29              
  Lines        1439     1442       +3     
  Branches      342      343       +1     
==========================================
+ Hits         1167     1170       +3     
  Misses        218      218              
  Partials       54       54              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chrisgrieser chrisgrieser deleted the dev branch January 3, 2025 12:31
@chrisgrieser
Copy link
Contributor Author

chrisgrieser commented Jan 3, 2025

@skovhus Sorry to bother you again, but it seems this PR did not fix the issue.

The problem described in #1064 still occurs, and shellcheck does not become active in a zsh file, even if passing bashIde.shellcheckArguments = "--shell=bash". I assume the problem is that the LS somehow still does not enable shellcheck until it is a bash file?

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.

shellcheck: forcing to work with zsh does not work
3 participants