Allow users to set keys to be bind, before sourcing the script, using environment variables#123
Allow users to set keys to be bind, before sourcing the script, using environment variables#123ericbn wants to merge 1 commit into
Conversation
|
|
||
| * `HISTORY_SUBSTRING_SEARCH_VICMD_DOWN_KEYS` is a global array that defines the | ||
| VI mode keys to be bind to the `history-substring-search-down` command. | ||
| Its default value is `('j')`, which is the code for the `j` key. |
There was a problem hiding this comment.
I think there's lots of repeating words in the above paragraphs, but I tried to keep the bullets layout. Maybe this could be reformatted as a table?
There was a problem hiding this comment.
True. I think the repetition is fine. If you really want to reformat this as a table, please make sure to do the same for all other configuration variables too.
There was a problem hiding this comment.
Let's keep the repetition here then, as you're fine with it. I can try a table layout later in a separate PR...
| fi | ||
| if (( ! $+HISTORY_SUBSTRING_SEARCH_PREFIXED )); then | ||
| typeset -g HISTORY_SUBSTRING_SEARCH_PREFIXED='' | ||
| fi |
There was a problem hiding this comment.
I'm also fixing the issue with WARN_CREATE_GLOBAL, as I commented here: #108 (comment)
There was a problem hiding this comment.
PR #119 seems simpler than this implementation. I'm leaning toward merging it instead.
There was a problem hiding this comment.
True, that implementation is simpler. Let me update the PR and use that here.
There was a problem hiding this comment.
The typeset -g VAR="${VAR:=value}" construct does not seem correct to me, as is does the assignment twice: at ${VAR:=value} and also at typeset -g VAR="value", so I changed it to what seems more correct.
| receive globally unique search results only once, then use this | ||
| configuration variable, or use `setopt HIST_IGNORE_ALL_DUPS`. | ||
|
|
||
| The following variables must be overridden before having loaded this script |
There was a problem hiding this comment.
I'm hesitant about the "must" requirement here: it's helpful to provide these key arrays to simplify the new-user experience but currently, we don't require users to bind our widgets. They can choose to simply load our plugin and never actually use its functionality. This kind of basic "null" use-case should be preserved, I feel.
There was a problem hiding this comment.
Okay, so all these properties will be empty by default, in order to preserve the current "null" use-case. Let me change that and update the README accordingly.
sunaku
left a comment
sunaku
left a comment
There was a problem hiding this comment.
Thanks for this PR. Please see my feedback in the code review.
|
@sunaku, PR updated. |
ericbn
changed the title
This will save users from some common boilerplate code in their own .zshrc file. Fixes zsh-users#107
|
Nit: CONTROL is such a weird way to spell Ctrl. |
|
It was being spelled as "Control" and "C" and also "^" before. I tried to unify it to a common format ("CONTROL"), as we have other keys in all caps too (e.g. "UP", "DOWN") in the same document. Fun fact: my keycaps actually spell it as "Control" :- ) @sunaku, what do you prefer? |
3 participants
This will save users from some common boilerplate code in their own
.zshrcfile.Fixes #107