Emacs: neil.el should work with compound commands

[agzam]

• This PR corresponds to Neil.el doesn't work if neil-executable-path is set to command with arguments, e.g. clj -M:neil #245

• I have updated the CHANGELOG.md file with a description of the addressed issue.

• I have considered whether I should add more tests covering the code I've changed.

[borkdude]

Could you maybe explain the fix? Why is there a specific regex about clj/clojure?
Does another command like bb -Sdeps '...' -m babashka.neil not work?
In lsp-mode you can define lsp-clojure-custom-server-command to be a list of strings, would something similar not work here?

https://github.com/emacs-lsp/lsp-mode/blob/c3be413f8545a26ad0334e1dd2b7e65c448d9f22/clients/lsp-clojure.el#L41

[agzam]

Hmm... good point. So, basically, I'm trying to resolve the executable path, i.e. if it's set to "clj -M:neil", it should become something like "/usr/bin/clj -M:neil". But (executable-find "clj -M:neil") => nil - it is not smart enough. So I need to resolve only the first part and then stitch the cmd-line string together.

I suppose I can make it work for any generic command - trying to resolve the first part of it.

[a13]

@agzam Just in case, shell-command-to-string works even if I use a command name without the full path, it has to be in $PATH though (as well as with executable-find case), so the solution looks a bit overcomplicated to me.

[agzam]

the solution looks a bit overcomplicated to me.

executable-find pattern is quite common for this kind of cases, shell-commands may fail if:

• The executable isn't in the current PATH
• The PATH env in Emacs differs from your shell's PATH

executable-find helps with:

• minimizing ambiguity, resolving to the actual binary file, avoiding shell-level abstractions like aliases, functions, or built-ins, symlinked executables, systems with multiple Python/Node/etc. versions
• you get the same binary regardless of PATH order or shell configuration

when you run (shell-command-to-string "foo"), where "foo" can't be found, the error message may differ on different systems with different shells - accurately detecting exact cause and reporting about the failure to the user requires more work.

I don't think it's an overcomplication worth simplifying.

[a13]

The executable isn't in the current PATH

in this case, most likely it isn't in exec-path either

The PATH env in Emacs differs from your shell's PATH

exec-path-from-shell helps with that a bit

minimizing ambiguity, resolving to the actual binary file, avoiding shell-level abstractions like aliases, functions, or built-ins, symlinked executables, systems with multiple Python/Node/etc. versions

Ok, but if we want to avoid the ambiguity and other shell-related problems, shell-command isn't the best solution either, why not call-process for example?

you get the same binary regardless of PATH order or shell configuration

or you don't get it at all :)

An alternative solution would be to use two defcustoms - one for the command itself and another for (optional) args. Like some other Emacs packages do.

[agzam]

Like I said: "I don't think it's an overcomplication worth simplifying.". If you disagree, please make another PR, we can even hold off merging this one until then. If it makes simpler, you can even do it in a nested PR.

You do know how the saying goes: "Talk is cheap, send patches". I already spent a good chunk of my Saturday to fix a problem you pulled out of thin air, if the proposed solution is still not bringing any joy, please do improve it, I have no emotional attachment to this code, nor do I have any privilege to dictate how it should be structured.

In short - I probably had good reasons to put that check in place, because things were failing for me otherwise. I don't remember the details anymore, but if you can prove that piece is really not needed - the best way to prove that is with code. Otherwise we may get bogged down debating for a long time about various ways things may or may not fail.

[borkdude]

Thanks a lot @agzam for your time and contributions!

I think I should have asked first why @a13 wasn't using babashka to run neil which makes the most sense and then we probably wouldn't even have ended up here, but it's good to have this fixed anyway.

[agzam]

@a13 upon revisiting my own comment, I just realized how awfully angry it may sound for the reader on the other side. I'm sorry, never was my intent. I just want to emphasize, there's no reluctance from me for any change - if you feel anything can be improved, of course, I will happily review any proposals, I just don't like mincing words when something can be better expressed with code. The last thing I would ever want is to make anyone feel discouraged. Cheers.