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

add zsh support, remove extra output on macOS #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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