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

Fix running in shells with -u/-o nounset option set #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntninja
Copy link

@ntninja ntninja commented May 16, 2021

No description provided.

ntninja added a commit to ntninja/oh-my-zsh that referenced this pull request May 16, 2021
@rupa
Copy link
Owner

rupa commented May 20, 2021

interesting

  1. can you tell me a bit about use case? this thing's designed to work at top-level, and i've never found much use for it in scripting. it's never occurred to me to run day-to-day shell in hardmode - so just curious if that's what you're doing, or I just don't get the use case.

  2. if we support this (in not opposed - although a bit nonplussed) i'd prefer to do it by handling all the pertinent vars toward the top of the script, e.g. FOO=${FOO:-bar}. would like your thoughts on that. maybe you tried that first and didn't like it?

@ntninja
Copy link
Author

ntninja commented May 20, 2021

can you tell me a bit about use case? this thing's designed to work at top-level, and i've never found much use for it in scripting. it's never occurred to me to run day-to-day shell in hardmode - so just curious if that's what you're doing, or I just don't get the use case.

I just like seeing error messages when mistyping variable names. Except for having submit patches to OMZ every now and then, there is nothing “hard mode” at all about this. 😛

if we support this (in not opposed - although a bit nonplussed) i'd prefer to do it by handling all the pertinent vars toward the top of the script, e.g. FOO=${FOO:-bar}. would like your thoughts on that. maybe you tried that first and didn't like it?

I tend to avoid writing it that way to reduce namespace pollution a bit, but if you prefer it that way I don't mind updating the PR accordingly.

@rupa
Copy link
Owner

rupa commented May 26, 2021

ah fair, namespace pollution. Let me think about that a bit - I was definitely more concerned with avoiding it when I first wrote this tool, than I am now - but it's still definitely on my mind.

thanks for letting me know the details of your use case - makes sense, just never really occured to me :)

@unsignedzero
Copy link

unsignedzero commented Sep 26, 2021

Hello.

I don't know if this is worth adding onto this PR (since I found it from the same issues as above) but 213 is missing a -z flag in the conditional as well as line 217, see below. I have the same issue and opted to use +x over +set which fixes the issue as well. Hopefully this gets merged in soon.

[ -z "${_Z_NO_RESOLVE_SYMLINKS+x}" ] || _Z_RESOLVE_SYMLINKS="-P"

if type compctl >/dev/null 2>&1; then
    # zsh
    [ "-z ${_Z_NO_PROMPT_COMMAND+x}" ] || {

Changes

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.

3 participants