Improved completions [for #145] #128
Conversation
…option from a group is already chosen
There was a problem hiding this comment.
Cool stuff!
Grouped flags and completing abbreviations is great. Replacing the few tab outliers with spaces is a good fix. And clear commit messages.
I'm going to pull the maintainer card and ask you to bring back the double quotation mark convention. I know how you feel… because I have it opposite
Looks like the abbreviations' completion needs some work. And because abbreviations and expansions can both have single quotes, double quotes, and colons, it's going to be tricky.
completions/_abbr
Outdated
| '--help[Show the manpage.]' \ | ||
| '(-v --version)'{-v,--version}'[Show the current version.]' \ |
There was a problem hiding this comment.
revert pls. I hear your UX expectation, as well as the implicit "these aren't commands so they shouldn't be in cmds". But in the special cases of help and version I prefer to eagerly surface their existence, instead of only after -
There was a problem hiding this comment.
Gotcha, I will revert it.
Since _values has a similar syntax to _arguments, we could group the related commands the same way, but this may be unnecessary?
There was a problem hiding this comment.
Since _values has a similar syntax to _arguments, we could group the related commands the same way, but this may be unnecessary
Say more?
There was a problem hiding this comment.
Sure, an example would be:
{a,add}"[Add a new abbreviation.]" \
{c,clear-session}"[Erase all session abbreviations.]" \
# ...
{R,rename}"[Rename an abbreviation.]" \
{version,-v,--version}"[Show the current version.]"In this case I don't think it would ne needed to use the parentheses syntax, i.e.
"(R rename)"{R,rename}"[Rename an abbreviation.]"for excluding related commands/options, since in this context only one can be used and suggested.
There was a problem hiding this comment.
I think we can click resolve on this. If you agree @Farid-NL, go ahead and do it
|
|
||
| return ret | ||
| _abbr |
There was a problem hiding this comment.
Is there precedent for wrapping this in a function? (it's been a while since I studied others' completion files) Is it advantageous?
Any risk of confusing the shell by giving it the same name as the file? at one point I had a function in zsh-abbr.zsh called _abbr, and that tripped up the completion system.
There was a problem hiding this comment.
Is there precedent for wrapping this in a function? (it's been a while since I studied others' completion files) Is it advantageous?
There's a lot of discrepancies for the structure of completions as far as I can see. So my reasoning was that return is valid inside functions.
I've read mainly these completions:
/usr/share/zsh/functions/Completion/Unix/_git/usr/share/zsh/functions/Completion/Unix/_pkgadd/usr/share/zsh/functions/Completion/Unix/_rar/usr/share/zsh/vendor-completions/_docker
In some of those files they don't even return anything or wrap the logic in a main function _<command_name>(), some of them do. It seems that when there's some complex logic behind the completions they tend to use a wrapper function.
Any risk of confusing the shell by giving it the same name as the file? at one point I had a function in zsh-abbr.zsh called _abbr, and that tripped up the completion system.
None as far as I know if it's only used in the completion file with a wrapper function, this is used by git and docker completions (from the examples above)
There was a problem hiding this comment.
I unwrapped the main logic and place the helper functions above it. It seems to work fine. If you want we can do this.
There was a problem hiding this comment.
Undecided.
I feel like zsh completions files are already so opaque, simplify as much as possible so give the dev community examples that aren't impossible to understand. And it's been a while since I looked, but I'm pretty sure I've seen a bunch of others that don't wrap. On the other hand, my convention is to wrap shell script file contents in a function!
Let's come back to it after everything else is finalized.
There was a problem hiding this comment.
I think we can click resolve on this. If you agree @Farid-NL, go ahead and do it
|
Thanks for being game to talk this through and try different things. |
Glad I can help! |
|
|
Sorry for the delay, some life related stuff. I have a question regarding some examples to test # for ' and "
abbr -S 'a"b"c'=d
abbr -S "a'b'c"=eHow would you normally erase/rename these abbreviations? What would you put exactly to erase/rename them? I'm getting or |
No apologies necessary, take all the time you need.
|
|
So, with that limitation in mind what would be the next steps to green light the PR? Right now just certain characters (#118) doesn't seem to work in abbreviations, expansions with those characters work however (they are shown correctly in the completion's description). # Display de completion properly the action (rename/erase) works.
abbr -S a:=b
abbr -S b=a:
abbr -S a="b="
abbr -S we="asd^asd\"hey\""
abbr -S wa="as\!hey"
# Display the completion properly but it won´t rename/erase it
abbr -S 'a"b"c'=d
abbr -S "a'b'c"=e
|
|
Haven't forgotten! Planning this for v6 (no estimated date yet) Closes #145 |
|
@Farid-NL I'm taking advantage of GitHub's "allow edits from maintainers" feature to force-push your branch. I'm rebasing it off my |
|
Scratch that it doesn't let me force push 🥲 |
|
Pushed updates to resolve a couple threads: And I noticed one important completions bug: Currently the completions will suggest options after the subcommand arguments abbr erase myabbreviation # TAB shows `-`, another TAB shows flagsbut zsh-abbr has a design limitation where all flags have to go before the subcommand name: # good
abbr --dry-run erase myabbreviation
# errors
abbr erase myabbreviation --dry-runI see two possibilities: modify the completions so that flags only come before subcommand, or update Removing the limitation would be cool. But if changing the completion is easy, that's probably the way to go for now. I don't know when I'll have time to work on either. Not soon. If you or someone else reading this is up for looking into it, awesome |
Hi! 👋
The main goal of this PR is to provide a list of abbreviations when calling the
eraseandrenamecommands.There were other improvements I made a long the way though
Using double quotes only when necessary
Reasoning
Even though could be a performance improvement between using single or double quotes... this might be my OCD triggered.
Separate options from commands
Reasoning
Normally when pressing Tab you expect just the list of commands, but not the options i.e.
--help. These will only show up when typing-→ Tab (for short and long flags) or--→ Tab (for long flags).Demo
List abbreviations when using
eraseandrenamecommandsReasoning
Maybe you don't use an abbreviation anymore, so instead of listing the abbreviations manually, an then type it into the
eraseorrenamecommand, you can search for it wit the help of the completion system.Features
Demo
Group related options and set exceptions when using a particular group
Reasoning
There could be multiple options for a specific context, for example:
[<SCOPE>]:User (-U || --user), Session (-S || --session)[<TYPE>]:Regular (-r || --regular), Global (-g || --global)[<QUIET_MODE>]:Quiet (-q || --quiet), Quieter (-qq || --quieter)So when one is chosen, the other options for the same context should not be shown.
Example:
-Sis used, then--session,-Uand--userare not shown.Demo