Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch to default shell instead of bash #499

Closed
Creator54 opened this issue Jul 31, 2022 · 8 comments · Fixed by #549
Closed

switch to default shell instead of bash #499

Creator54 opened this issue Jul 31, 2022 · 8 comments · Fixed by #549

Comments

@Creator54
Copy link

xplr 0.19.0
currently ":!" gives me bash want to use my default shell instead

@sayanarijit
Copy link
Owner

Hi, you can easily overwrite the default config. See https://xplr.dev/en/configuration

xplr.config.modes.builtin.action.key_bindings.on_key["!"].messages = {
  { Call = { command = "zsh", args = { "-i" } } },
  "ExplorePwdAsync",
  "PopMode",
}

@Creator54
Copy link
Author

Creator54 commented Jul 31, 2022

Hi, you can easily overwrite the default config. See https://xplr.dev/en/configuration

xplr.config.modes.builtin.action.key_bindings.on_key["!"].messages = {
  { Call = { command = "zsh", args = { "-i" } } },
  "ExplorePwdAsync",
  "PopMode",
}

thanks, I am currently using this. However wouldn't it be better if xplr read the $SHELL and used it by default and have the option like here to change if anyone wanted ?

@sayanarijit
Copy link
Owner

I guess that's the expected behavior. However, I believe a lot of zsh users (like me) are using oh-my-zsh and there's an issue that require some workaround to get xplr play nicely with it.

Though unrelated to xplr and zsh, I thought it'd be better if we solve this issue first before defaulting to $SHELL. Feel free share your thoughts.

@Creator54
Copy link
Author

I guess that's the expected behavior. However, I believe a lot of zsh users (like me) are using oh-my-zsh and there's an issue that require some workaround to get xplr play nicely with it.

Though unrelated to xplr and zsh, I thought it'd be better if we solve this issue first before defaulting to $SHELL. Feel free share your thoughts.

Yup tried zsh same for me too, hope it gets fixed soon.

@SaphiraKai
Copy link

In my opinion, completely ignoring the user's default shell setting just to prevent one edge case with one shell is too broad.

As a much less heavy-handed workaround, you could check for the existence of $HOME/.oh-my-zsh/ and override to bash if it's found.

@SaphiraKai
Copy link

Here's a simple implementation of that I whacked together:

--- init.lua
+++ auto-shell-init.lua
@@ -1828,4 +1828,19 @@
 }
 
+-- Workaround https://github.com/ohmyzsh/ohmyzsh/issues/9859
+local function has_ohmyzsh()
+  local path = os.getenv("HOME").."/.oh-my-zsh"
+  local ok, err, code = os.rename(path, path)
+  if not ok then if code == 13 then return true end end
+  return ok, err
+end
+
+local function shell()
+  local shell = os.getenv("SHELL")
+  if shell == "/bin/zsh" and has_ohmyzsh() then
+    return "bash" else return shell
+  end
+end
+
 -- The builtin action mode.
 --
@@ -1839,5 +1854,5 @@
         messages = {
           "PopMode",
-          { Call0 = { command = "bash", args = { "-i" } } },
+          { Call0 = { command = shell(), args = { "-i" } } },
           "ExplorePwdAsync",
         },

When the issue is resolved, both functions can be deleted and the call to shell() can be replaced with os.getenv("SHELL").

@sayanarijit
Copy link
Owner

In my opinion, completely ignoring the user's default shell setting just to prevent one edge case with one shell is too broad.

Good point.

As a much less heavy-handed workaround, you could check for the existence of $HOME/.oh-my-zsh/ and override to bash if it's found.

I wouldn't, because workarounds like this often lead to hacky and unmaintainable code. For e.g. imagine fish, nushell, etc. and countless other shells require workarounds like this too. Since bash is a dependency for xplr, and it is heavily tested, defaulting to bash seemed like a safer option.

However, I think we should try defaulting to $SHELL and see what happens. Maybe users facing this issue will come up with a good solution that we've been missing.

@SaphiraKai
Copy link

Sounds like a good idea to me, crowd-sourcing ideas and implementations is one of the main benefits of open-source, after all.

sayanarijit added a commit that referenced this issue Nov 30, 2022
sayanarijit added a commit that referenced this issue Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants