[vim/vim] runtime(sh): set shellcheck as the compiler for supported shells (PR #16311)
Luca Saccarola
Hi,
I think we should be setting shellcheck has the compiler for every shellcheck supported shell.
https://www.shellcheck.net/wiki/SC1071.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/16311
Commit Summary
- ab4b5ae runtime(sh): set shellcheck as the compiler for supported shells
File Changes
(1 file)
- M runtime/ftplugin/sh.vim (2)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gary Johnson
> Hi,
> I think we should be setting shellcheck has the compiler for every shellcheck
> supported shell.
>
> • https://github.com/vim/vim/pull/16311.patch
> • https://github.com/vim/vim/pull/16311.diff
I wrote a command and function to use shellcheck as a 'makeprg'
a while back and set this so that Vim could properly parse the
output:
let &makeprg = "shellcheck -f gcc -x"
Regards,
Gary
vim-dev ML
> I think we should be setting shellcheck has the compiler for every shellcheck
> supported shell.
>
> • https://github.com/vim/vim/pull/16311.patch
> • https://github.com/vim/vim/pull/16311.diff
I wrote a command and function to use shellcheck as a 'makeprg'
a while back and set this so that Vim could properly parse the
output:
let &makeprg = "shellcheck -f gcc -x"
Regards,
Gary
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Enno
While at it, shouldn't one check for executable('shellcheck') as well?
In its absence, fall back to
CompilerSet makeprg=bash\ -n CompilerSet errorformat=%f:\ line\ %l:\ %m
if g:is_bash is set
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
Doesn't shellcheck need the "-f gcc" and maybe "-x" options?
This is set when you do :compiler shellcheck. That command will load the $VIMRUNTIME/compiler/shellcheck which if we look inside of it will set the errorformat and makeprg correctly.
While at it, shouldn't one check for executable('shellcheck') as well?
Yes, it is probably a good idea
In its absence, fall back to
I don't know. I think we should make a new compiler for bash and it is probably good for another PR
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
@saccarosium pushed 1 commit.
- 1185316 runtime(sh): set shellcheck as the compiler for supported shells
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -44,15 +44,21 @@ if (has("gui_win32") || has("gui_gtk")) && !exists("b:browsefilter")
let b:undo_ftplugin ..= " | unlet! b:browsefilter"
endif
-if get(b:, "is_bash", 0)
+let s:is_sh = get(b:, "is_bash", 0)
I suppose this should have been testing for b:is_sh instead?
> @@ -44,15 +44,21 @@ if (has("gui_win32") || has("gui_gtk")) && !exists("b:browsefilter")
let b:undo_ftplugin ..= " | unlet! b:browsefilter"
endif
-if get(b:, "is_bash", 0)
+let s:is_sh = get(b:, "is_bash", 0)
+let s:is_bash = get(b:, "is_bash", 0)
+let s:is_kornshell = get(b:, "is_kornshell", 0)
Those 3 should probably be instead:
⬇️ Suggested change-let s:is_kornshell = get(b:, "is_kornshell", 0) +let s:is_kornshell = get(b:, "is_kornshell", get(g:, "is_kornsh", 0))
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Luca Saccarola
@saccarosium pushed 1 commit.
- 2f06bbe runtime(sh): set shellcheck as the compiler for supported shells
You are receiving this because you are subscribed to this thread.
Luca Saccarola
@saccarosium commented on this pull request.
> @@ -44,15 +44,21 @@ if (has("gui_win32") || has("gui_gtk")) && !exists("b:browsefilter")
let b:undo_ftplugin ..= " | unlet! b:browsefilter"
endif
-if get(b:, "is_bash", 0)
+let s:is_sh = get(b:, "is_bash", 0)
Yes, good catch!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Enno
@saccarosium : Since this is still open, how about falling back to compiler bash if shellcheck not found (and is_bash set ?)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
@saccarosium pushed 1 commit.
- c063cb2 runtime(sh): set shellcheck as the compiler for supported shells
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
> +let s:is_sh = get(b:, "is_sh", 0) +let s:is_bash = get(b:, "is_bash", 0) +let s:is_kornshell = get(b:, "is_kornshell", get(g:, "is_kornsh", 0))
Do the introduced s: variables need to stay defined after
the sourcing?
In syntax/sh.vim, there are is_kornshell queries and aren't
is_kornsh ones. Look for is_kornshell with both gets?
In syntax/sh.vim, g:is_sh and g:is_bash are recognised.
Look for these too?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Aliaksei Budavei
>
+if executable('shellcheck') && (s:is_sh || s:is_bash || s:is_kornshell)
Test the acquired variables and then do a system call?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Aliaksei Budavei
> if !exists('current_compiler')
compiler shellcheck
endif
let b:undo_ftplugin .= ' | compiler make'
+elseif s:isbash
⬇️ Suggested change
-elseif s:isbash +elseif s:is_bash
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Aliaksei Budavei
> + if !exists('current_compiler')
+ compiler bash
+ endif
+ let b:undo_ftplugin .= ' | compiler make'
⬇️ Suggested change
- if !exists('current_compiler')
- compiler bash
- endif
- let b:undo_ftplugin .= ' | compiler make'
+ let b:undo_ftplugin ..= ' | compiler make'
Shouldn't the restoring of compiler be also made conditional
here and at lines 62-65?
There is also an out-of-style use of .= at line 65.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Aliaksei Budavei
Ping the file owner, @dkearns.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -44,19 +44,30 @@ if (has("gui_win32") || has("gui_gtk")) && !exists("b:browsefilter")
let b:undo_ftplugin ..= " | unlet! b:browsefilter"
endif
-if get(b:, "is_bash", 0)
+let s:is_sh = get(b:, "is_sh", 0)
⬇️ Suggested change
-let s:is_sh = get(b:, "is_sh", 0) +let s:is_sh = get(b:, "is_sh", get(g:, "is_sh", 0))
> @@ -44,19 +44,30 @@ if (has("gui_win32") || has("gui_gtk")) && !exists("b:browsefilter")
let b:undo_ftplugin ..= " | unlet! b:browsefilter"
endif
-if get(b:, "is_bash", 0)
+let s:is_sh = get(b:, "is_sh", 0)
+let s:is_bash = get(b:, "is_bash", 0)
⬇️ Suggested change
-let s:is_bash = get(b:, "is_bash", 0) +let s:is_bash = get(b:, "is_bash", get(g:, "is_bash", 0))
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Luca Saccarola
@saccarosium pushed 1 commit.
- 5620561 runtime(sh): set shellcheck as the compiler for supported shells
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
> +let s:is_sh = get(b:, "is_sh", get(g:, "is_sh", 0)) +let s:is_bash = get(b:, "is_bash", get(g:, "is_bash", 0)) +let s:is_kornshell = get(b:, "is_kornshell", get(g:, "is_kornsh", 0))⬇️ Suggested change
-let s:is_sh = get(b:, "is_sh", get(g:, "is_sh", 0)) -let s:is_bash = get(b:, "is_bash", get(g:, "is_bash", 0)) -let s:is_kornshell = get(b:, "is_kornshell", get(g:, "is_kornsh", 0)) +let s:is_kornshell = get(b:, "is_kornshell", get(g:, "is_kornshell", 0))
Let me ask again:
Do the introduced s: variables need to stay defined after
the sourcing?
In other words:
unlet s:save_cpo s:is_sh s:is_bash s:is_kornshell
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Luca Saccarola
@saccarosium pushed 1 commit.
- d117d4d runtime(sh): set shellcheck as the compiler for supported shells
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Andrea Pappacoda
@Tachi107 commented on this pull request.
>
+if (s:is_sh || s:is_bash || s:is_kornshell) && executable('shellcheck')
What about is_posix?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.