Fish tab completions #763
Fish tab completions #763
Conversation
There was a problem hiding this comment.
@John-Goff thanks for adding fish shell support. These completions look great. I left a couple of suggestions for you.
In the meantime, I’ll pull this down and give it a try.
| ### Fish | ||
|
|
||
| Completions must go in the user defined `$fish_complete_path`. By default, this is `~/.config/fish/completions` | ||
|
|
There was a problem hiding this comment.
For consistency could we keep the bundle completion file named exercism_completion.fish.
There was a problem hiding this comment.
We could do that, but the final file has to have the same name as the executable. I figured it would be easier to have it named that way from the start, rather than requiring users to rename it themselves. I foresee at least a few people dragging and dropping rather than using the command line.
| # Download | ||
| complete -f -c exercism -n "__fish_use_subcommand" -a "download" -d "Downloads and saves a specified submission into the local system" | ||
| complete -f -c exercism -n "__fish_seen_subcommand_from download" -s e -l exercise -d "the exercise slug" | ||
| complete -f -c exercism -n "__fish_seen_subcommand_from download" -s h -l help -d "help for download" |
There was a problem hiding this comment.
We can probably do without tab completion for the help flag, as it will repeat the information you have already added for each option. Thoughts?
There was a problem hiding this comment.
these completions fire when a user types exercism <command> -<tab> (note the dash). It's only there for completeness, I created these mostly by copying the output of exercism help <command>.
|
|
||
| # Help | ||
| complete -f -c exercism -n "__fish_use_subcommand" -a "help" -d "Shows a list of commands or help for one command" | ||
| complete -f -c exercism -n "__fish_seen_subcommand_from help" -a "configure download help open submit troubleshoot upgrade version workspace" |
There was a problem hiding this comment.
These fire after typing exercism help, it's useful to be able to tab through the list of commands you can run help on.
|
Has anyone been able to test these? If I need to make any changes, please let me know. |
@John-Goff I had an opportunity to test the first time around, but didn't test entirely. I will take another look. Thank you for the detailed responses. |
There was a problem hiding this comment.
@John-Goff sorry for the long review time. It has been a busy last few months for me. I hope you are still with us and interested in pushing this forward. I tested the completions and things work as expected - the Fish Shell is alright.
I left a comment regarding the sequence completion for the timeout flag. Otherwise this is looks good to me. Thanks!
shell/exercism.fish
Outdated
|
|
||
| # Options | ||
| complete -f -c exercism -s h -l help -d "show help" | ||
| complete -f -c exercism -l timeout -a "(seq 0 100 100000)" -d "override default HTTP timeout" |
There was a problem hiding this comment.
The tab completion for possible timeout values is cool. But I found the possible options a bit overwhelming - it filled my terminal. Is this a common practice you’ve seen in fish? If so, I recommend shortening the possible value range.
Removing the range entirely has a funky behavior where if I tab after the timeout flag it tab completes to some random file. Possibly why you added the sequence?
There was a problem hiding this comment.
I can definitely shorten the range. I agree there's potentially too many options to tab through. Your analysis is spot on, I added it to prevent the files autocompleting there. Files autocompleting is fish's default behaviour, and most programs do this if there's no available options to add. I thought some range of numbers to be better than nothing, if for no other reason than to show that it's possible. I'll shorten the range.
There was a problem hiding this comment.
Is it possible in fish to document a value as it is in zsh? If so wouldn't it make sense to only have most commonly used timouts of 10, 30, 60 seconds, 5 and 10 minutes?
In zsh such a completion list would roughly look like:
10 -- 10 seconds
30 -- 30 seconds
60 -- 1 minute
300 -- 5 minutes
600 -- 10 minutes
I consider such a completion much better than a plain list of values that no-one knows what they do without canceling the completion and looking them up in the documentation first.
There was a problem hiding this comment.
This is indeed possible and I have updated the completions to include this. Thanks for the suggestion, I was really unsure what to put for timeout values.
There was a problem hiding this comment.
In fish this looks like
➤ exercism --timeout=
…-timeout=10 (10 seconds) …-timeout=30 (30 seconds) …-timeout=60 (1 minute) …-timeout=300 (5 minutes) …-timeout=600 (10 minutes)
|
@nywilken No problem and thanks for your review! |
|
I've left another comment on the timeout range inline. |
|
Apologies I didn't see this update before now. Thank you for your review, I have updated the completions to incorporate your suggestions. |
|
Thank you @John-Goff! I'm going to pull this in, and if we find any other improvements we can do them in a second round ✨ |
|
@John-Goff please use one of the github keywords to automatically close a referenced issue when the PR has been merged so it's easier :) thanks for the work! |
#758
I tried to copy the zsh and bash completions as closely as I could, but I'd appreciate if someone else could review these. Preferably someone who's more familiar with how the bash and zsh completions should preform than I am!