Add support for ale option to override default shell used by ale #2167

stegmanh · 2018-12-28T17:06:29Z

This PR adds the g:ale_shell and g:ale_shell_argument options. Allowing the user to customize which shell ale will use to execute commands and allowing the user to specify any additional arguments they wish to pass to the execute function.

The reasoning for this change it to address an issue that came up in #1968. The TL;DR is that there are some instances where Neovim attempts to run a command via zsh and it fails due to odd argument passing, which can be fixed by just using bash to run the commands.

By default the g:ale_shell option is set to the &shell vim variable. And the g:ale_shell_argument option is set to -c as was default in ale.

Files changed in this PR:

autoload/ale/job.vim - Add support for g_ale_shell and g:ale_shell_argument commands and use those when running commands
doc/ale.txt - Add documentation for new options
test/* - Fix tests

RyanSquared · 2018-12-28T17:09:25Z

autoload/ale/job.vim

@@ -188,11 +194,11 @@ function! ale#job#PrepareCommand(buffer, command) abort
        return 'cmd /s/c "' . l:command . '"'
    endif

-    if &shell =~? 'fish$\|pwsh$'
-        return ['/bin/sh', '-c', l:command]
+    if g:ale_shell =~? 'fish$\|pwsh$'


i think this should be documented in the section for g:ale_shell

RyanSquared · 2018-12-28T17:10:35Z

autoload/ale/job.vim

-    if &shell =~? 'fish$\|pwsh$'
-        return ['/bin/sh', '-c', l:command]
+    if g:ale_shell =~? 'fish$\|pwsh$'
+        return ['/bin/sh', g:ale_shell_arguments, l:command]


please revert this to be hardcoded '-c' to avoid conflicts with non-sh-compatible shells; because sh is standardized we know we can pass -c to it.

RyanSquared · 2018-12-28T17:12:14Z

doc/ale.txt

+  This variable is used to determine which shell ale will use to execute 
+  commands. This variables defaults to the value of the vim option '&shell'
+  which corresponds to the $SHELL environment variable. For example 
+  if `$SHELL == '/bin/bash'`, but you want to use zsh, set `g:ale_shell = '/bin/zsh'.`


add in a comment to suggest using g:ale_shell_arguments if overriding this, as g:ale_shell_arguments can pull incompatible flags from &shellcmdflag even if the user changes shell

…ish and pwsh environments

stegmanh · 2018-12-28T17:37:13Z

Thanks for reviewing that @RyanSquared! I just pushed a PR that addressed those comments. I think I might need to take a look at the appveyor logs though. That build appears to be failing

RyanSquared · 2018-12-28T17:42:31Z

It breaks due to: https://github.com/w0rp/ale/blob/3ec20a730d1c6485ab28704f930a61c6ba426b27/autoload/ale/job.vim#L194

@w0rp both lists and strings are returned from ale#job#PrepareCommand - is that intentional, and if so, can that behavior be changed to be consistent?

w0rp

I think instead of replacing the built in variables with configurable ones which default to the Vim variables, make it so the default for the shell option is empty or undefined, and defining the option completely replaces all of the behavior, including the checks for cmd on Windows, fish, etc. Update the documentation to note that if you change the option, you're essentially on your own after that. You can default the shell options to the Vim variable.

If we do all of that, that will allow people to replace their shell on Windows, to use some hack or other to get fish to work, etc.

w0rp · 2018-12-29T16:59:21Z

autoload/ale/job.vim

        return ['/bin/sh', '-c', l:command]
    endif

-    return split(&shell) + split(&shellcmdflag) + [l:command]
+    return [g:ale_shell] + split(g:ale_shell_arguments) + [l:command]


It might be wise to stick with split() here. It's mentioned in Vim or NeoVim documentation somewhere, but I can't remember where right now.

w0rp · 2018-12-29T17:03:19Z

It breaks due to:

ale/autoload/ale/job.vim

Line 194 in 3ec20a7
return 'cmd /s/c "' . l:command . '"'

@w0rp both lists and strings are returned from ale#job#PrepareCommand - is that intentional, and if so, can that behavior be changed to be consistent?

Yes. Windows needs Strings, Unix needs Lists. Windows is weird.

w0rp · 2018-12-29T17:05:01Z

The deal with Lists not working on Windows is that the Windows shell has no general support for quoting shell arguments. For cmd, argument quoting is handled by the programs, instead of by the shell itself. This means that quoting arguments can sometimes result in say literally "foo" instead of foo for "foo". Windows is weird.

stegmanh · 2018-12-30T01:40:07Z

Thanks for the review @w0rp. Ill make those changes

RyanSquared · 2019-01-02T21:27:01Z

Relevant: #2175

stegmanh · 2019-01-03T00:15:32Z

Alright I think these changes are in the direction you were talking about @w0rp.

I was running into issues testing locally with the readfile command not being able to read a tmp file. Hopefully it doesn't come up in travis, but if it does Ill investigate further

w0rp

Looks good. I'll take this, and I'll fix some of the formatting, which looks like it's already a bit wrong. I'll probably update the documentation so it's a bit less verbose, and I might make the variables unset instead of v:null.

w0rp · 2019-01-03T21:01:19Z

Cheers! 🍻

stegmanh · 2019-01-03T21:34:31Z

Cool! Thanks for merging this & helping with improving the PR. I'll keep an eye out for your PR so I know how to do unset and better formatting in the future.

🍻

hibachrach · 2019-01-04T06:26:36Z

I may be wrong here, but this may have broken tests on AppVeyor.

RyanSquared · 2019-01-04T14:36:06Z

Correct:

    (5/5) [EXECUTE] (X) ['/foo/bar', '/c', 'foobar'] should be equal to ['/foo/bar', '-c', 'foobar']

w0rp · 2019-01-04T14:49:58Z

Yeah, I broke it.

w0rp · 2019-01-04T14:52:22Z

I pushed a commit which should fix that now. I'll keep an eye on the test results.

Holden added 3 commits December 7, 2018 18:04

Add initial support for settings to overwrite ale shell

cbc029b

Merge branch 'master' of github.com:stegmanh/ale

57a0470

add tests for ale_shell option

c499825

RyanSquared reviewed Dec 28, 2018

View reviewed changes

address PR feedback by fixing docs and hardcoding bash to use -c on f…

3ec20a7

…ish and pwsh environments

fix tests on windows

8550fa6

w0rp requested changes Dec 29, 2018

View reviewed changes

vendion mentioned this pull request Jan 2, 2019

Work around shell oddities #2175

Closed

change ale to use value of g:ale_shell regardless of what it is

5f613a5

w0rp approved these changes Jan 3, 2019

View reviewed changes

w0rp merged commit 7919db0 into dense-analysis:master Jan 3, 2019

nospam2998 mentioned this pull request Feb 4, 2022

Linting/Checking fails on rust files with some shells. #1968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ale option to override default shell used by ale #2167

Add support for ale option to override default shell used by ale #2167

stegmanh commented Dec 28, 2018

RyanSquared Dec 28, 2018

RyanSquared Dec 28, 2018

RyanSquared Dec 28, 2018

stegmanh commented Dec 28, 2018

RyanSquared commented Dec 28, 2018

w0rp left a comment

w0rp Dec 29, 2018

w0rp commented Dec 29, 2018

w0rp commented Dec 29, 2018

stegmanh commented Dec 30, 2018

RyanSquared commented Jan 2, 2019

stegmanh commented Jan 3, 2019

w0rp left a comment

w0rp commented Jan 3, 2019

stegmanh commented Jan 3, 2019

hibachrach commented Jan 4, 2019

RyanSquared commented Jan 4, 2019

w0rp commented Jan 4, 2019

w0rp commented Jan 4, 2019

Add support for ale option to override default shell used by ale #2167

Add support for ale option to override default shell used by ale #2167

Conversation

stegmanh commented Dec 28, 2018

RyanSquared Dec 28, 2018

Choose a reason for hiding this comment

RyanSquared Dec 28, 2018

Choose a reason for hiding this comment

RyanSquared Dec 28, 2018

Choose a reason for hiding this comment

stegmanh commented Dec 28, 2018

RyanSquared commented Dec 28, 2018

w0rp left a comment

Choose a reason for hiding this comment

w0rp Dec 29, 2018

Choose a reason for hiding this comment

w0rp commented Dec 29, 2018

w0rp commented Dec 29, 2018

stegmanh commented Dec 30, 2018

RyanSquared commented Jan 2, 2019

stegmanh commented Jan 3, 2019

w0rp left a comment

Choose a reason for hiding this comment

w0rp commented Jan 3, 2019

stegmanh commented Jan 3, 2019

hibachrach commented Jan 4, 2019

RyanSquared commented Jan 4, 2019

w0rp commented Jan 4, 2019

w0rp commented Jan 4, 2019