anki-code / xontrib-sh

Paste and run commands from bash, zsh, fish, tcsh in xonsh shell.
MIT License
54 stars 5 forks source link

feat: Allow custom shell name to be manually specified #2

Closed eugenesvk closed 3 years ago

eugenesvk commented 3 years ago

I've added one feature:

  1. Custom shell name can be manually specified, e.g. !bash <cmd> or !b <cmd> instead of being auto-detected with ! <cmd>. Each matching option (full name or first letter) is configured via a separate environmental variable ($XONTRIB_SH_MATCHFULL or $XONTRIB_SH_MATCHFIRST), both enabled by default

and removed the other feature originally submitted from this PR, let me know if you reconsider this WSL support. By the way, I've also excluded the WSL bash from matching when a user is outside a WSL instance

  1. Can run a WSL shell (rather useful on Windows) with, e.g. !fw <cmd> for fish in WSL (if it's included in the list of shells)

Only tested this on Windows with bash/fish/pwsh on xonsh/0.9.26, but seems to be working fine :)

anki-code commented 3 years ago

Hi @eugenesvk! Thank you for implementing this!

  1. Added the comments to the code.
  2. Could you please describe the use case around WSL? As I understand if you're in WSL you don't need to do wsl -e. And if you're in Windows what is the profit of running wsl -e?
eugenesvk commented 3 years ago
  1. Added the comments to the code.

Don't see any comments

  1. Could you please describe the use case around WSL?

To be able to run some WSL shell commands while working within a Windows native xonsh instance without having to launch a separate WSL xonsh instance

As I understand if you're in WSL you don't need to do wsl -e.

Yes, that's correct, within WSL you're just like within a native Linux instance, so you can access e.g. fish by simply running fish, no need to invoke wsl.exe

And if you're in Windows what is the profit of running wsl -e?

Within Windows only bash is accessible via bash.exe (there is 'C:\Windows\System32\bash.exe'), so if you want to execute a command in another shell installed within WSL, you'd need to invoke it via wsl.exe -e <shell> ..., where '-e'="Execute the specified command without using the default Linux shell."

anki-code commented 3 years ago

Don't see any comments

Ah, I forgot to submit them. Here they are above.

To be able to run some WSL shell commands while working within a Windows native xonsh instance without having to launch a separate WSL xonsh instance

I think this use case is very special for Windows and out of this xontrib. By default we assume that the shells are exist in the current environment. By doing wsl -e you jump from one environment to another. I suggest to remove this from PR.

eugenesvk commented 3 years ago

By default we assume that the shells are exist in the current environment.

Well, not on Windows :) — your default shell check condition is true for bash even outside of WSL precisely because there exists 'C:\Windows\System32\bash.exe'

for s in _shells:
  if which(s): # this is true for bash even outside of WSL since its output is `'c:\\windows\\system32\\bash.EXE'`
  _installed_shells.append(s)

This is actually where the idea for wsl -e came from, I wanted to make it slightly more consistent (although bash is still a first-class citizen since it doesn't need that extra w to run)

To make the default assumption correct you'd actually need to add some kind of which(s) return value check and don't append the _installed_shells if it returns 'c:\\windows\\system32\\bash.EXE'

If you want to remove the commit with the WSL access feature, should we also exclude bash by performing the check desribed above? Then I'll add it to the first commit and resubmit a PR without the 2nd commit (though think it might still be nice if your xontrib could acess any shell)

anki-code commented 3 years ago

Now looks good to me, thanks! Two small things:

  1. I suggest to make the windows bash checking more universal i.e. if 'system32\\bash.exe' in which('bash').lower(), you know.

  2. If the shell name doesn't exist or we've got the empty list at the end of checking show the error. It works silently and this could frustrate:

    image

eugenesvk commented 3 years ago
  1. shortened to bash.exe to avoid any win bashes
  2. added errors when no shell is installed and when an explicitly passed name doesn't match an installed shell (though we don't differentiate between a no match and not installed to avoid testing every shell in the list when a user passed a specific one)
anki-code commented 3 years ago

Big step forward, thanks! The walrus operator is cool :) but we want to be on the same wave with xonsh that support Python 3.6+. Please back to the country side from the city :)

eugenesvk commented 3 years ago

:=((

anki-code commented 3 years ago

Awesome! Many thanks!