Skip to content

main: Add ZSH_HIGHLIGHT_DIRS_BLACKLIST #496

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

Merged
merged 2 commits into from
Feb 18, 2018

Conversation

phy1729
Copy link
Member

@phy1729 phy1729 commented Jan 21, 2018

Closes #379.

This may fail on symlinks, but I think this should cover most cases. I placed the check below the -L and -e tests so once the file is completely typed it will still highlight as path.

Please test that the speed with this change is usable if you've run into #379. If things are still too slow the check can be moved up (or we could test that the word is no longer being typed with $CURSOR and have a cache).

I'll add docs after we get some reports and know how $ZSH_HIGHLIGHT_DIRS_BLACKLIST will act.

@phy1729
Copy link
Member Author

phy1729 commented Jan 21, 2018

Seems ${name:|arrayname} was added in zsh 5.0.0.

@phy1729
Copy link
Member Author

phy1729 commented Feb 18, 2018

Merging this to master as an experimental interface. Hopefully this will make testing easier. If positive feedback ensues, the X_ can be dropped.

@phy1729 phy1729 merged commit 15e288a into zsh-users:master Feb 18, 2018
@phy1729 phy1729 deleted the dirs-blacklist branch March 13, 2018 03:16
@phy1729 phy1729 added this to the 0.7.0 milestone Mar 17, 2018
@phy1729
Copy link
Member Author

phy1729 commented Jul 20, 2019

We got some feedback in #528 and moved the blacklist check to prior to any syscalls. @danielshahaf do you think that is sufficient to drop the X_ prefix before 0.7.0 or should we leave it this round?

@danielshahaf
Copy link
Member

@phy1729 Let's see. The feature was added in 0.6.0-89-g6713727 (6713727). On the one hand, that's 18 months ago (a very long time, so far as exposure to users of master is concerned); on the other hand, the feature hasn't appeared in any tagged release yet. Both the functionality and the implementation are straightforward and unlikely to change.

In light of all these, I guess we can just drop the X_ prefix? We can always do a 0.7.0-rc1 release to solicit some more feedback. (Actually, we can tag rc1 right now, can't we?)

If you want to make it a zstyle, by the way, that'd work for me — see #362; so long as we put a version number or a format number at the head of the style, that'd raise no forward compatibility issues — but of course, simply renaming the parameter is a lot easier.

What will the transition be? Will the code continue to honour $X_ZSH_HIGHLIGHT_DIRS_BLACKLIST if it is set? What about if $X_ZSH_HIGHLIGHT_DIRS_BLACKLIST and $ZSH_HIGHLIGHT_DIRS_BLACKLIST are both set?

@phy1729
Copy link
Member Author

phy1729 commented Jul 21, 2019

I would leave changing to a zstyle to later when the rest of the config will be changed as well.

Regarding the transition, I was thinking something like the patch below. The notice in zsh-syntax-highlighting.zsh will be nicer to hit as it's prior to drawing the prompt. The notice in main-highlighter.zsh will catch if X_ZSH_HIGHLIGHT_DIRS_BLACKLIST is set after we'd sourced. In either case the code configured the new variable, so the blacklist continues working.

I would like to put this (and the X_ removal) in now, tag -rc1, and then remove the warnings prior to release in a week. Those who follow master tend to upgrade frequently. Although, I have no objection to lengthening the amount of time this stays in to catch anyone on vacation or otherwise AFK.

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index 91be6ca..838ab2c 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -355,6 +355,12 @@ _zsh_highlight_highlighter_main_paint()
     '!' # reserved word; unrelated to $histchars[1]
   )
 
+  if (( $+X_ZSH_HIGHLIGHT_DIRS_BLACKLIST )); then
+    print >&2 'X_ZSH_HIGHLIGHT_DIRS_BLACKLIST is deprecated. Please use ZSH_HIGHLIGHT_DIRS_BLACKLIST.'
+    ZSH_HIGHLIGHT_DIRS_BLACKLIST=($X_ZSH_HIGHLIGHT_DIRS_BLACKLIST)
+    unset X_ZSH_HIGHLIGHT_DIRS_BLACKLIST
+  fi
+
   _zsh_highlight_main_highlighter_highlight_list -$#PREBUFFER '' 1 "$PREBUFFER$BUFFER"
 
   # end is a reserved word
diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index df8a1aa..6b75a5e 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -432,6 +432,12 @@ zmodload zsh/parameter 2>/dev/null || true
 # Initialize the array of active highlighters if needed.
 [[ $#ZSH_HIGHLIGHT_HIGHLIGHTERS -eq 0 ]] && ZSH_HIGHLIGHT_HIGHLIGHTERS=(main)
 
+if (( $+X_ZSH_HIGHLIGHT_DIRS_BLACKLIST )); then
+  print >&2 'X_ZSH_HIGHLIGHT_DIRS_BLACKLIST is deprecated. Please use ZSH_HIGHLIGHT_DIRS_BLACKLIST.'
+  ZSH_HIGHLIGHT_DIRS_BLACKLIST=($X_ZSH_HIGHLIGHT_DIRS_BLACKLIST)
+  unset X_ZSH_HIGHLIGHT_DIRS_BLACKLIST
+fi
+
 # Restore the aliases we unned
 eval "$zsh_highlight__aliases"
 builtin unset zsh_highlight__aliases

@danielshahaf
Copy link
Member

I would leave changing to a zstyle to later when the rest of the config will be changed as well.

+1

I would like to put this (and the X_ removal) in now, tag -rc1, and then remove the warnings prior to release in a week. Those who follow master tend to upgrade frequently. Although, I have no objection to lengthening the amount of time this stays in to catch anyone on vacation or otherwise AFK.

That all sounds good, with the caveat you foresaw: I don't know if leaving it in for just a week would be enough. How often do those plugin managers auto-update us?

  • if (( $+X_ZSH_HIGHLIGHT_DIRS_BLACKLIST )); then

Should we check not only whether the array is defined but also whether it's not empty? I guess the condition is good as-is, but thought I'd ask.

  • print >&2 'X_ZSH_HIGHLIGHT_DIRS_BLACKLIST is deprecated. Please use ZSH_HIGHLIGHT_DIRS_BLACKLIST.'

Let's prefix the output with zsh-syntax-highlighting: .

  • ZSH_HIGHLIGHT_DIRS_BLACKLIST=($X_ZSH_HIGHLIGHT_DIRS_BLACKLIST)

What if both variables are set? I don't think we should just overwrite the value of $ZSH_HIGHLIGHT_DIRS_BLACKLIST in that case.

  • unset X_ZSH_HIGHLIGHT_DIRS_BLACKLIST

Thanks.

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.

Disable path checking by directory
2 participants