Update EmuHawkMono.sh #4594
Update EmuHawkMono.sh #4594
Conversation
Added: * check against useless `"$0"`, * `export GTK_PREFIX=`, and * shellcheck directives against warnings; redirected status messages to `stderr`; and removed `exec`.
|
I complied with what seems to be the style guide, but that isn't the standard style guide (notably, the use of double-quotes instead of single-quotes and the quoted |
YoshiRulz
left a comment
YoshiRulz
left a comment
There was a problem hiding this comment.
I'm a bit confused by this PR. Did you just throw it into a linter and apply every suggestion?
| *"EmuHawkMono.sh");; | ||
| *"/bin/"*"sh") | ||
| # Very bad way to detect /path/to/shell | ||
| echo "I don't know where I am! Could you run me as \"/path/to/EmuHawkMono.sh\"?" |
There was a problem hiding this comment.
It should support being renamed via symlinking.
There was a problem hiding this comment.
Does this now support something like ln -s /opt/bizhawk/EmuHawkMono.sh ~/.local/bin/emuhawk?
There was a problem hiding this comment.
Yes. It will just think you're running it with a shell if that executable's name ends in sh, but not .sh. I don't know how else to better detect a shell, since bash for some reason uses its fullly-qualified path as "$0" when a script is sourced.
Assets/EmuHawkMono.sh
Outdated
| exec mono EmuHawk.exe "$@" | ||
| echo "(it seems EmuHawk is already running, NOT capturing output)" >& 2 | ||
| mono EmuHawk.exe "$@" | ||
| return 0 |
There was a problem hiding this comment.
What's wrong with exec? And at least return $?.
There was a problem hiding this comment.
Exec for a command is undefined behaviour in sh, which you're targeting. You are right about return "$?", though.
There was a problem hiding this comment.
Exec for a command is undefined behaviour in
sh, which you're targeting.
Looks defined to me, and I couldn't find anyone proscribing against it. Which shell doesn't support it?
There was a problem hiding this comment.
Huh. I thought I'd read that in the publication somewhere. Oh well.
Also, that link leads to the main page, not the specific page you likely wanted.
There was a problem hiding this comment.
Also, it looks like exec will only look for non-builtins, though this behaviour can be mimicked by just using ./command. It seems you can replace what I did with exec -- # command if you want to.
| "arch"|"artix"|"manjarolinux") libpath="/usr/lib";; | ||
| "fedora"|"gentoo"|"nobaralinux"|"opensuse") libpath="/usr/lib64";; | ||
| "nixos") libpath="/usr/lib"; printf "Running on NixOS? Why aren't you using the Nix expr?\n";; | ||
| "nixos") libpath="/usr/lib"; echo "Running on NixOS? Why aren't you using the Nix expr?" >& 2;; |
There was a problem hiding this comment.
This is not an error, this codepath is theoretically supported, though I have no idea if it works.
There was a problem hiding this comment.
Also I'm in the habit of always using printf because of echo's footguns.
There was a problem hiding this comment.
Correct, it is not an error. However, status messages should always go to stderr, so not to conflate with actual output.
There was a problem hiding this comment.
The footguns you mention only really happen with arbitrary variables. If one were to pass in -n from a variable, echo would get confused. However, when printing a simple string, it's better to use the simplest method.
YoshiRulz
added
the
Meta
|
No, I didn't "just throw it into a linter", though I do have a linter in my IDE. I noticed some minor issues, and thought the simplest way to get them fixed was to simply open a PR. |
|
What's this style guide you're referring to? |
|
I guess there isn't a strict “standard style guide”, just “what you'll generally see code written like online/in books”, thus being what more people are used to. If you mean your style guide, it's what I inferred from how you wrote the code in that file. |
2 participants
Added:
"$0",export GTK_PREFIX=, andreplaced
printfwithecho; redirected status messages tostderr; added--safeguard for builtins when necessary; and removedexec.Check if completed: