use which on formatter command

[zaucy]

This allows my formatter command to work on Windows and Linux without having to edit my config between machines. Example:

[[language]]
name = "typescript"
formatter = { command = './node_modules/.bin/prettier', args = ["--parser", "typescript"] }

I'm unsure if this is breaking expected behaviour, but to me this seems like a non-breaking improvement especially for Windows users who often have to switch between operating systems.

[Masynchin]

You can get rid of .unwrap()'s by converting Result to Option. I wrote this code sample as an example of it, haven't test it though. You can use it if it is improves readability.

let fmt_cmd = |formatter| which::which(&formatter.command)
    .ok()
    .and_then(|cmd| cmd.into_os_string().into_string())

if let Some((fmt_cmd, fmt_args)) = self
    .language_config()
    .and_then(|c| c.formatter.clone())
    .and_then(|formatter| fmt_cmd(formatter).map(|cmd| (cmd, formatter.args.clone())))

[zaucy]

Thanks! I removed the unwrap calls.

[kirawi]

I think #5832 addresses this.

[zaucy]

I personally would prefer to not run through my shell (especially on Windows), but I can see the advantage with environment variables expansion etc.

I think #5538 (comment) is a pretty reasonable instead of forcing use of the editor shell.

[clo4]

I personally would prefer to not run through my shell (especially on Windows)

Hmm, yeah I feel the same way - I'd rather not launch my shell just to run the formatter, that seems like unnecessary overhead. I keep my shell launching fast so it's no big deal to me, but there's no reason to run it just for this, and for people that have slower setups it'll definitely be noticeable.

The other issue with launching a shell is that it will very likely cause issues with other people's shell setups. Some people have interactive prompts when they launch a shell regardless of whether it's interactive or not (while working for Fig I saw a surprising amount of people with this 😔). Also seems like it might (?) cause issues with ZSH users that unconditionally prepend their path in ~/.zshenv, for example with Python virtualenvs.

IIRC Helix now has its own reusable variable expansion, right? Could we use that here?

[kirawi]

There is, but it only takes in a Path. You would also have to parse the arguments being passed, and correctly replace them with the expanded paths.

https://github.com/helix-editor/helix/blob/master/helix-core/src/shellwords.rs could maybe be used towards that.

[archseer]

I personally would prefer to not run through my shell

I agree here, if shell expansion is desired the end user can always manually invoke the shell: zsh -c 'formatter $VAR' etc

[archseer]

Does this also address #5538?

[pascalkuthe]

Does this also address #5538?

I don't think so arguments are not shell expanded here. We could check if an argument starts with a tilde and expands appropriately (or full shell expansion ala https://crates.io/crates/shellexpand).

That seems orthogonal to me tough, this PR makes formatters behave the same as LSP servers which I think is the right thing for now. If we do endup adding such a feature I thing we should do the same thing for LSP servers.

[pascalkuthe]

ah missed somethinmg

[zaucy]

cargo check is passing locally for me, but the CI is failing. I don't think it's related to my PRs contents. Rebasing against master to see if it is fixed.