-
-
Notifications
You must be signed in to change notification settings - Fork 49
Various fixes and improvements to fish shell completion #86
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
Conversation
Escape every tldr page name to prevent fish syntax errors. This fixes the problem in which special characters in fish were not escaped when fish shell completion was added in tldr-pages#17, because at the time none of the pages in <https://github.com/tldr-pages/tldr> had names that were syntactically special in fish. This meant that escaping was unnecessary. However, as a result of <tldr-pages/tldr#7530>, which added the files `[.md` and `[[.md`, the fish shell autocompletion will now cause a syntax error in fish whenever autocompletion triggers. This syntax error fills stderr with long error messages and prevents the fish autocompletion from running. This problem is fixed by escaping characters like `[`, which is done in this commit. See also: <tldr-pages/tldr#7530>, tldr-pages#17
When using `tldr` to show a tldr-page, specifying a file path instead of a name of a page is not allowed and will simply fail with the message `This page doesn't exist yet!`. However, when using fish autocompletion, it will display file and directory names along with the names of tldr-pages, even though using those suggestions results in an error. This commit tells fish that it should not complete filenames. Documentation: <https://fishshell.com/docs/current/cmds/complete.html>
`tldr` accepts a `-r`/`--render` argument, which is used like `tldr --render <PATH>` or `tldr -r <PATH>`. The current behavior for fish autocompletion of the `--render` option added `PATH` as a *literal value* for completion, meaning that it would suggest `PATH` itself as a valid path. Instead, fish should autocomplete paths for us, and the value `PATH` should be removed as a literal autocompletion option. Documentation: <https://fishshell.com/docs/current/cmds/complete.html>
`tldr` fish autocompletion is now functional, but the autocompletion will sometimes suggest things that are invalid, such as: - Suggesting more tldr-page names after autocompleting the first, even though you can only get one tldr-page at a time. - Recommending incompatible option combinations, like suggesting `--update`, `--help`, and `--render` at the same time. - Suggesting multiple file paths after specifying `--render`, even though that isn't valid syntax. This commit resolves these problems by checking to see what values have already been entered. This makes the suggestions more "intelligent". Documentation: <https://fishshell.com/docs/current/cmds/complete.html>, <https://fishshell.com/docs/current/completions.html#completion-own>.
The documentation for `complete` lists `-x` as an alias for `-rf`. This allows for some options to be removed because their functionality is already enabled by other flags. > -x or --exclusive > > Short for -r and -f. Source: <https://fishshell.com/docs/current/cmds/complete.html> This change is just refactoring. No functionality is different compared to the last commit.
|
I've made additional improvements to fish autocompletion in addition to the critical bug fix. Now, it won't repeatedly suggest things over and over (even though its suggestions were invalid) and the code is tidier. I believe this PR is ready to be merged, especially because it fixes a bug that caused fish-shell completion to be broken since March 10, 2021. |
|
Hi all! This thread has not had any recent activity. |
|
Does anyone have the ability to review this PR? It's been finished for a while and fish shell completion is broken without it. |
|
Hi all! This thread has not had any recent activity. |
|
Is |
johnflavin-fw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for doing this. I wish I could approve/merge but I am a lowly user just like you.
I was preparing to submit my own PR to fix this issue before I found yours. I've left comments about some of the changes I had made that you didn't. Might be easier to combine our efforts and fix everything at once.
johnflavin-fw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more comments.
|
Pinging people who have recently approved / merged PRs: @MasterOdin @owenvoke @CleanMachine1 Any additional changes needed to merge this? |
|
I'm happy to merge this once you (@johnflavin-fw) and @superatomic are aligned with changes (@johnflavin-fw please approve once you're satisfied). I don't use fish or know the syntax for this file, so I cannot give feedback on the suggested changes. |
Suggested by https://github.com/johnflavin-fw This change is just refactoring. No functionality is different compared to the last commit.
Suggested by @johnflavin-fw on GitHub
Fish autocompletion will sort and remove duplicates for us. Suggested by @johnflavin-fw on GitHub
|
There's a few more changes that I'd like to make, but after that, let's finally get this merged! |
This change is just refactoring. No functionality is different compared to the last commit.
`complete -c tldr -f` used to be within the `if test -d "$HOME/.tldrc/tldr/pages"` code block. This meant that if `~/.tldrc` didn't exist (most likely due to `tldr --update` not having been run yet), then `tldr` autocompletion would start autocompletion paths instead of nothing. This commit fixes this by moving `complete -c tldr -f` to the outside of the `if` statement.
The `--list` option was introduced in tldr-pages#42 and was requested in tldr-pages#22 & tldr-pages#36
This adds fish autocompletion for the options introduced in tldr-pages#46.
The description for `--platform` doesn't need to contain all acceptable values, since the autocompletion provides these options anyway.
johnflavin-fw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! I've tested it locally and haven't seen any issues.
Suggested by @johnflavin-fw on GitHub
|
I want to try to make the command completion be more dynamic in how it suggests certain flags; for example, the |
|
Sounds good @superatomic. Just ping here in the PR again once you'd like me to merge, but if you do end up making larger changes, I'd probably ask @johnflavin-fw to do another re-review. |
Use `__fish_is_first_arg` instead of `__fish_use_subcommand`. This prevents options like `--version`, `--list`, `--help`, `--update`, and `--clear-cache` from being suggested when other options like `--osx` have already been used, as that would be an invalid mix of options.
Using the phrase "this help" makes sense when running `tldr --help`, but not when using autocompletion. This is fixed by this commit.
Prevent autocompletion of things like `--help` after `--linux`, and etc.
|
Okay, I'm made all of the improvements that I can make. I'm not sure if builtin functions exist for implementing anything more complex. @johnflavin-fw it would be great if you did another review. I made a lot of changes that restricted where different options could be autocompleted, and it's very possible that I accidentally made a regression somewhere. Otherwise, if everything's working okay, let's finally get this PR merged! 🎉 |
The first line in `__tldr_positional_no_os` had to be indented.
Some completions used the `-x` flag (an alias of `-rf`), but shouldn't have been using the `-r` flag (which is for arguments that require a parameter after the option, like `--platform` and `--render`). Documentation: <https://fishshell.com/docs/current/cmds/complete.html>
|
I've tried it out. Everything seems to work as far as I can tell. LGTM |
This change is just refactoring. No functionality is different compared to the last commit.
|
I made a small refactoring by renaming LGTM! |
|
@MasterOdin Ready to merge |
|
@MasterOdin Do we have to do anything else to merge this? We both agree that this PR is ready to be merged. |
|
Apologies, this completely slipped off my radar (again). Thanks for the efforts @superatomic and @johnflavin-fw on this! |
Co-authored-by: John Flavin <flavinj@gmail.com>
What does it do?
This pull request fixes a fatal bug in the fish shell autocompletion that caused it to crash with a syntax error.
It also makes improvements to the functionality of the fish shell autocompletion. Fixes #87.
Functionality before these changes:
[TAB]represents pressing the tab key and is not actually visible.$is the prompt,tldr [TAB]is user input, and the rest is error output.Functionality after these changes:
[TAB]represents pressing the tab key and is not actually visible.$is the prompt,tldr [TAB]is user input, and the rest is output.Escape every tldr-page name to prevent fish syntax errors 45cd3d5
This fixes the problem in which special characters in fish were not
escaped when fish shell completion was added in #17, because at the time
none of the pages in https://github.com/tldr-pages/tldr had names that
were syntactically special in fish. This meant that escaping was
unnecessary.
However, as a result of tldr-pages/tldr#7530*,
which added the files
[.mdand[[.md, the fish shell autocompletionwill now cause a syntax error in fish whenever autocompletion triggers.
This syntax error fills stderr with long error messages and prevents the
fish autocompletion from running. This problem is fixed by escaping
characters like
[, which is done in this commit.*Update:[.mdwas actually introduced before[[.mdin tldr-pages/tldr#5205 instead of tldr-pages/tldr#7530. This means that fish autocompletion has been broken since Mar 10, 2021.Prevent filenames from displaying in fish autocomplete c4cfc4d
When using
tldrto show a tldr-page, specifying a file path instead ofa name of a page is not allowed and will simply fail with the message
This page doesn't exist yet!.However, when using fish autocompletion, it will display file and
directory names along with the names of tldr-pages, even though using
those suggestions results in an error.
This commit tells fish that it should not complete filenames.
Require a filepath after
-rin fish autocomplete 6cfe15atldraccepts a-r/--renderargument, which is used liketldr --render <PATH>ortldr -r <PATH>.The current behavior for fish autocompletion of the
--renderoptionadded
PATHas a literal value for completion, meaning that it wouldsuggest
PATHitself as a valid path.Instead, fish should autocomplete paths for us, and the value
PATHshould be removed as a literal autocompletion option.
Documentation for
complete(fish builtin)Make fish-shell autocompletion smarter a839565
tldrfish autocompletion is now functional, but the autocompletionwill sometimes suggest things that are invalid, such as:
though you can only get one tldr-page at a time.
--update,--help, and--renderat the same time.--render, eventhough that isn't valid syntax.
This commit resolves these problems by checking to see what values have
already been entered. This makes the suggestions more "intelligent".
Remove redundant options in fish completion 2dc6b41
The documentation for
completelists-xas an alias for-rf.This allows for some options to be removed because their functionality
is already enabled by other flags.
This change is just refactoring. No functionality is different compared
to the last commit.
Why the change?
The first change is very important because fish autocompletion is broken without it.
The other commits resolve problems where the autocompletion suggests values that are not actually valid.
All of these changes make the fish autocompletion more useful.
How can this be tested?
This modifies the shell autocompletion and not the main source code. I believe this can only be tested manually by installing the shell completion file in fish and verifying that it works.
I can verify that it works on my machine.
Every modification entirely uses functions that are fish builtins, so the completions should work for every instance of fish.
Where to start code review?
All changes to code are in autocomplete/complete.fish. Five separate changes to improve the shell completion have been made.
The CHANGELOG.md was also updated as this PR fixes a fatal bug in fish autocompletion.
Documentation: https://fishshell.com/docs/current/cmds/complete.html,
https://fishshell.com/docs/current/completions.html#completion-own.
Relevant tickets?
tldr-pages/tldr#5205, tldr-pages/tldr#7530, #17, #87
These changes are explicitly licensed under the MIT License.