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 plugin for shells with no_unset (aka set -u) and ksh_arrays shell options set #713

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

Conversation

ntninja
Copy link

@ntninja ntninja commented Nov 2, 2022

No description provided.

@ntninja ntninja changed the title Fix plugin for shells with no_unset shell option (aka set -u) set Fix plugin for shells with no_unset (aka set -u) and ksh_arrays shell options set Nov 5, 2022
Copy link

@ShayanAhmad ShayanAhmad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@REALERvolker1
Copy link

Just going to comment on this: set -u is useful in interactive contexts, and it would be nice to have it in autosuggestions.

@z0rc
Copy link

z0rc commented May 28, 2024

set -u isn't useful in any way in interactive shell, see discussion at romkatv/powerlevel10k#2652.

@LucasLarson
Copy link

set -u isn't useful in any way in interactive shell

is incorrect and the latest message in romkatv/powerlevel10k #2652 is incorrect. When you source a function like this:

$ cat ./foo && . ./foo
break_nounset() {
  printf 'foo%s\n' "${this_variable_does_not_exist}"
}

it returns different results based on user settings

$ set -u && break_nounset
break_nounset:1: this_variable_does_not_exist: parameter not set
$ set +u && break_nounset
foo

Having set -u in an interactive shell is invaluable while dogfooding works in progress.

@z0rc
Copy link

z0rc commented May 29, 2024

That's the whole point. nounset is useful in script and isn't useful in interactive session. If you decided that that nounset must be enabled during interactive session for some deliberate reason, it's on you to deal with consequences.

I must emphasize, zsh-autosuggestions is designed to run in interactive session, it's expected to have sane defaults in interactive session, set -u isn't part of said defaults.

@LucasLarson
Copy link

I must emphasize, zsh-autosuggestions is designed to run in interactive session, it's expected to have sane defaults

It appears that much of the codebase does allow for a user to work with it interactively, even if set -o nounset/set -u/setopt nounset is activated. In 2019, for example, the codebase was modified so that users with set -o sh_word_split/set -y/setopt sh_word_split in their interactive shell can keep easy access (4cd210b70d).

I see plenty of benefit in publishing changes that would allow this plugin to degrade gracefully and work for users coming with various options set.

What’s the benefit of preventing this merge?

@JamesWidman
Copy link

JamesWidman commented Sep 6, 2024

+1 to @LucasLarson; no_unset is useful in interactive mode, and not just because it catches errors in functions: a parameter name can be typo'd (or erroneously unset) on the command prompt just the same as a parameter name in a script, and no_unset will prevent such an erroneous command on the prompt from being run. That's a very good thing if the erroneous command would produce irreversible effects or just consume a lot of time!

and for people who use unset in interactive mode, this PR does no harm.

there's now at least 4 different users who want this. Is there any reason not to merge?

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.

6 participants