Skip to content

Conversation

@TehBrian
Copy link

@TehBrian TehBrian commented May 4, 2022

This pull-request adds zsh support by:

  1. checking what shell is being used, and if it is zsh, using a different, zsh-compatible command to locate the MELEE_DIR directory; and,
  2. using eval to run the play command.

This PR also contains a second, smaller commit that removes the (eval):disown:1: no current job message when the script is run on macOS.

I'm quite a novice at scripting, so please tell me if anything looks wrong. I would greatly appreciate feedback. Specifically, the case block in the current_shell() is likely unnecessary, but I wasn't sure how I could strip a potential leading dash from ps's output.

@sudofox
Copy link
Owner

sudofox commented May 4, 2022

Thanks for the PR!

I'm asking around to find someone to verify this pull request for me since I can't afford a Mac lol. I'll get back to you soon hopefully!

Er, one more thing, the thing that checks the shell and whatnot does seem to add a bit of extra bulk, I might try to trim that down a teeny bit. I guess I ought to learn how to use zsh first since it's the hot new thing.

@TehBrian
Copy link
Author

TehBrian commented May 4, 2022

Alright, thanks for the consideration!

Er, one more thing, the thing that checks the shell and whatnot does seem to add a bit of extra bulk, I might try to trim that down a teeny bit.

Yup, Again, if you've got any ideas on how to trim some of that code down, I'd really appreciate it. I tried Googling a fair bit and there doesn't seem to be many good options that check the user's shell. For example, $SHELL reports the user's preferred shell, not the active one. ps seemed to be the best option that I could find, but I'm not sure why it sometimes prepends a dash to the process name.

@TehBrian
Copy link
Author

TehBrian commented May 4, 2022

Also, I think that the only reason that any of this is necessary is because #!/usr/bin/env bash is only telling melee.sh to define the function in bash, but the script inside the melee function is still run in the user's shell when the user calls it. Is there any way that we could run the melee function in bash?

@AlecsFerra
Copy link

News?

@Cyber-Cowboy
Copy link

Tried your merge on ubuntu, got:
melee.sh: Current shell is unsupported. >> Please use one of the following: bash, zsh
even though I use zsh. I think the problem is in this string $(ps -hp $$ | cut -d ' ' -f 8 | xargs) I tried to run it by hand in my terminal and got empty string. my ps output is:

 PID  TTY          TIME CMD
 122046 pts/6    00:00:00 zsh
 130080 pts/6    00:00:00 ps

@TehBrian
Copy link
Author

TehBrian commented Jul 4, 2022

News?

Haven't worked on this recently, but I'd appreciate any suggestions on how to improve it!

Tried your merge on ubuntu, got: melee.sh: Current shell is unsupported. >> Please use one of the following: bash, zsh even though I use zsh. I think the problem is in this string $(ps -hp $$ | cut -d ' ' -f 8 | xargs) I tried to run it by hand in my terminal and got empty string. my ps output is:

 PID  TTY          TIME CMD
 122046 pts/6    00:00:00 zsh
 130080 pts/6    00:00:00 ps

Yeah, that's odd. My pr definitely shouldn't be merged in its current state. There are likely compatibility issues scattered throughout.

If anyone wants to give zsh support a try, feel free to do so! I might give it another shot myself later when I have some spare time.

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 this pull request may close these issues.

4 participants