Skip to content

driver: ksharrays issue with $options #622

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

Closed
wants to merge 3 commits into from
Closed

driver: ksharrays issue with $options #622

wants to merge 3 commits into from

Conversation

austintraver
Copy link
Contributor

I personally use ksh-style arrays, but I noticed that this plugin doesn't work when that setting is enabled. Adding this one line of code allows this plugin to work for users who include "setopt ksharrays" in their .zshrc file.

I personally use ksh-style arrays, but I noticed that this plugin doesn't work when that setting is enabled. Adding this one line of code allows this plugin to work for users who include "setopt ksharrays" in their .zshrc file.
@danielshahaf
Copy link
Member

I don't think that's quite the right fix: it'll cause zsyh_user_options to be initialized with wrong values. Furthermore, the emulate -L zsh on line 103 already resets KSH_ARRAYS to the default value (off).

Presumably got a syntax error somewhere between line 88 (where you added the unsetopt) and line 102 (where the unsetopt already happens as part of emulate -L).

Could you propose a different fix?

@danielshahaf
Copy link
Member

By the way, the travis errors are due to #623; not your fault. Sorry about that.

@phy1729
Copy link
Member

phy1729 commented Jul 7, 2019

This appears to be an upstream bug.

% PROMPT='%% ' zsh -f
% print $#options
192
% setopt ksharrays
% print $#options 
3
% print $options
off

In addition, forcing the fallback path results in canonical_options being set to just (aliases) even though raw_options looks to be correct. (Of course onoff=${${=:-off on}[2-$?]} needs to be adjusted when running under ksharrays as well.)

@danielshahaf
Copy link
Member

This works:

% Src/zsh -fc 'setopt ksharrays; echo ${#${options[@]}}' 
192
% 

As to ${#options}: given that Src/zsh -fc 'setopt ksharrays; echo ${options}' prints off, I suspect ${#options} computes the length of the scalar "off". I don't know whether that's a bug or an expected effect of KSH_ARRAYS.

@phy1729
Copy link
Member

phy1729 commented Jul 7, 2019

Didn't think about [@]. If that works, this may suffice (when zsh/parameter is available).

diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index df8a1aa..36ab6e9 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -86,7 +86,7 @@ _zsh_highlight()
   # Before we 'emulate -L', save the user's options
   local -A zsyh_user_options
   if zmodload -e zsh/parameter; then
-    zsyh_user_options=("${(@kv)options}")
+    zsyh_user_options=("${(kv)options[@]}")
   else
     local canonical_options onoff option raw_options
     raw_options=(${(f)"$(emulate -R zsh; set -o)"})

@danielshahaf
Copy link
Member

Fixed the travis issue; the next push to this PR should be green.

@austintraver
Copy link
Contributor Author

Any other changes you guys would like to see before this gets pulled?

@danielshahaf
Copy link
Member

Could you fix the else branch as well?

Could you add comments (or a regression test)?

Copy link
Contributor Author

@austintraver austintraver left a comment

Choose a reason for hiding this comment

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

Updated as per request

Copy link
Member

@danielshahaf danielshahaf left a comment

Choose a reason for hiding this comment

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

Not ready to merge.

This is incorrect: $options doesn't exist in the else branch.

Sorry for the late answer; I didn't receive a notification.

else
local canonical_options onoff option raw_options
raw_options=(${(f)"$(emulate -R zsh; set -o)"})
canonical_options=(${${${(M)raw_options:#*off}%% *}#no} ${${(M)raw_options:#*on}%% *})
for option in $canonical_options; do
for option in $("${(kv)options[@]}"); do
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect: $options doesn't exist in the else branch.

@danielshahaf danielshahaf changed the title Patch syntax highlighting bug driver: ksharrays issue with $options Nov 11, 2019
@chitoku-k
Copy link

@austintraver (cc: @danielshahaf)
I found this issue can be affected by other plugins even when they just locally enable ksharrays. Do you happen to have time to fix them?

@danielshahaf
Copy link
Member

@chitoku-k I don't have time to fix this personally. If anyone has patches to propose, though, I'm all ears. For starters, can you check if merging just the first of the two changes in this PR makes a difference?

@austintraver
Copy link
Contributor Author

@austintraver (cc: @danielshahaf)
I found this issue can be affected by other plugins even when they just locally enable ksharrays. Do you happen to have time to fix them?

For me, this was a case of wanting a configuration set before I really realized what I was missing out on. After fiddling with the repo, and successfully making a version of this script that could handle ksharrays, eventually it broke again, and I just decided to throw my hands up and use the default array style.

I like it much better actually, so I've switched sides. I'm afraid my previous solution no longer works, it was broken a while back.

But that didn't feel like a helpful thing to say, so I ended up writing a solution tonight. Closing this pull request in favor of the continuation I wrote in #689

danielshahaf pushed a commit that referenced this pull request Mar 13, 2020
The «emulate» call isn't sufficient, since these lines are parsed before
it takes effect.

Fixes #689 (née #622).

See also #688 for preventing these gymnastics from being needed in the
first place.

See also junegunn/fzf#1924 for an inter-plugin
interaction that this probably fixes.
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