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 ZSH issues in brew-wrap - doesn't run brew-file #70

Merged
merged 1 commit into from
Dec 17, 2016
Merged

Fix ZSH issues in brew-wrap - doesn't run brew-file #70

merged 1 commit into from
Dec 17, 2016

Conversation

derimagia
Copy link
Contributor

You looped through "$@" in the latest change which because of the fact that zsh doesn't split on spaces and bash does it causes issues and it never actually did a "brew file init" like it should have.

Used emulate with local options instead

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 91.769% when pulling 982c3b8 on derimagia:master into 8893046 on rcmdnk:master.

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 9, 2016

Thank you for the pull request.

emulate is much better than temporally setopt (honestly I didn't know... thanks!)

About "$@", as long as I checked it works same in both bash and zsh (zsh 5.2 (x86_64-apple-darwin16.0) (macOS Sierra default), and zsh 5.2 (x86_64-apple-darwin15.0.0) (homebrew version)).
I checked some documents and all of them say they are same.
And your pull request doesn't include codes related to "$@", does it...?

About another point of local first=${w[0]},
I used temporally ksh option because some people use it as default,
so ${w[0]} could have different argument if the option is not explicitly set.

@derimagia
Copy link
Contributor Author

By "$@" I simply mean to put all the arguments in a single string and reply on setopt sh_word_split, the emulate fixes this since it emulates bash

emulate should reset it to the defaults, I just tried this:

echo $options[ksharrays]; setopt KSH_ARRAYS; echo ${options[ksharrays]}; function abc() { emulate -L zsh; echo $options[ksharrays]; setopt; }; abc; unsetopt KSH_ARRAYS;

and it verified that but Id have no issues with setting ksharrays if you want, it's just you need to unset it before you go to the default zsh autocomplete functions or you'll have errors

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 9, 2016

"$@" doesn't mean single word, but it represents separated words.
"$*" mean single word. And these rules are same in both Bash and Zsh.
For example, below document says so:

Unix Shells: Bash, Fish, Ksh, Tcsh, Zsh - Hyperpolyglot

sh_word_split options is for cases such:

$ x="a b"
$ touch $x

This makes "a b" file in zsh w/o sh_word_split, and "a" and "b" files in bash.
If sh_word_split is set, zsh also makes "a" and "b" files.

ksh_arrays/emulate -L zsh/sh_word_wplit don't change "$@" function:

$ setopt
interactive
monitor
shinstdin
zle
$ x=(0 1 2);echo ${x[1]};looptest () {for a in "$@";do echo $a;done;};looptest a b c
0
a
b
c
$
$ setopt ksharrays
$ setopt
interactive
ksharrays
monitor
shinstdin
zle
$ x=(0 1 2);echo ${x[1]};looptest () {for a in "$@";do echo $a;done;};looptest a b c
1
a
b
c
$ unsetopt ksharrays
$
$ setopt sh_word_split
$ setopt
interactive
monitor
shinstdin
shwordsplit
zle
$ x=(0 1 2);echo ${x[1]};looptest () {for a in "$@";do echo $a;done;};looptest a b c
0
a
b
c
$ unsetopt sh_word_split
$
$ emulate -L ksh
$ setopt
...
noksharrays           off
...
noshwordsplit         off
...
$ x=(0 1 2);echo ${x[1]};looptest () {for a in "$@";do echo $a;done;};looptest a b c
1
a
b
c
$
$ emulate -L zsh
$ setopt
interactive
localoptions
localpatterns
localtraps
monitor
shinstdin
zle
$ x=(0 1 2);echo ${x[1]};looptest () {for a in "$@";do echo $a;done;};looptest a b c
0
a
b
c
$

Maybe I'm missing something...?

@derimagia
Copy link
Contributor Author

Sorry, you're right I was mistaken. I took another list and it wasn't that line which was doing it, it was the if

aka [[ "aaa" != ^- ]] instead of [[ "aaa" =~ ^- ]]

I don't know why the first one works in ksh emulation (I'm looking at the options that are enabled with it set). If you know, i'd be curious

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 9, 2016

that code is wrong, sorry.

It should be:

if [[ ! "$a" =~ ^- ]];then ...

[[ "$a" != ^- ]] just compares a content of "$a" and a string ^-, so it doesn't show error, but is always true even for -starting options (unless the word of^-` is given as an argument...)

@derimagia
Copy link
Contributor Author

That's what I thought, but it was interesting it breaks zsh. I'm curious to what exactly the un-quoted ^ is doing when it's != in zsh

@derimagia
Copy link
Contributor Author

Nevermind, it's just doing file globbing

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 13, 2016

ok, how do you want to proceed?

If you can remove ($words[@]) related lines, I will merge.
You can include ! ... =~ ... correction, if you want.

Otherwise, I will close this PR and will commit these update by myself.

@derimagia
Copy link
Contributor Author

"$words[@]" is to do with the change to using emulate. You can revert it completely if you want (although I don't like the user of grepping the options when you can have local options). If you enable KSH_ARRAYS locally, you need to revert it before calling zsh autocomplete functions.

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 17, 2016

As discussed above, ($words[@]) and ("${words[@]") work exactly same for normal zsh configuration.
Why I did put these codes was that if people set ksharrays it could break the condition,
therefore it set ksharrays always to unify the usage.
It is same as in _brew, and both cases the script revert the opt if it is not the user setting.

But anyway, I just realized that in completion function these setopt values are reset to completion's default so that it doesn't need to think about user's setopt.
so, I'll merge it.

@rcmdnk rcmdnk merged commit 829fe13 into rcmdnk:master Dec 17, 2016
@derimagia
Copy link
Contributor Author

Emulate should also reset the values, fyi. I was saying that above - but you can try

setopt KSH_ARRAYS; emulate -L zsh; setopt

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 18, 2016

It resets setopt, but only in a function. Try:

 setopt KSH_ARRAYS;f () {emulate -L zsh;};f;setopt

This is why we can use it safely in the function.
Otherwise, we need to check setopt before changing it, and restore it as I did in the script.

@derimagia
Copy link
Contributor Author

Gotcha - you must be talking about something else. The great thing about emulate -L is the thing you just said...

If you're talking about reseting to the users defaults mid-function (Which I don't think I would ever do but if you really want to) you can just wrap it in a function like you have there.

@rcmdnk
Copy link
Owner

rcmdnk commented Dec 18, 2016

ah, sorry, I got it.
so it should be reverted before calling functions in which compadd is executed.
Fortunately it seems that it has worked even with ksh options (maybe no arrays in them), but yes I should be careful about it.

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