Closed superatomic closed 2 years ago
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. Are there any updates? Thanks!
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. Are there any updates? Thanks!
Is tldr-c-client
still maintained? I have been unable to use the client alongside fish-shell due to the broken autocompletion, and this fix hasn't gotten any review for months.
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.
There's a few more changes that I'd like to make, but after that, let's finally get this merged!
I want to try to make the command completion be more dynamic in how it suggests certain flags; for example, the --help
flag should block any other completions, and typing any flags or tldr-page names should prevent the --help
flag from appearing. It's a lot more complex, so if I can't do it in a reasonable way, we can merge this PR without them.
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.
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! :tada:
I've tried it out. Everything seems to work as far as I can tell. LGTM
I made a small refactoring by renaming __tldr_no_os_choice_opt_and_p
to __tldr_no_os_choice_opt_nor_p
to be a bit clearer. No functionality was changed in my last commit.
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!
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 https://github.com/tldr-pages/tldr-c-client/commit/45cd3d5cd5151e429ac3cd1e6f05081fc3edc23e
This fixes the problem in which special characters in fish were not escaped when fish shell completion was added in https://github.com/tldr-pages/tldr-c-client/pull/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 https://github.com/tldr-pages/tldr/pull/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.*Update:[.md
was actually introduced before[[.md
in https://github.com/tldr-pages/tldr/pull/5205 instead of https://github.com/tldr-pages/tldr/pull/7530. This means that fish autocompletion has been broken since Mar 10, 2021.Prevent filenames from displaying in fish autocomplete https://github.com/tldr-pages/tldr-c-client/commit/c4cfc4d8a89cacf82bcd208e36b69f416e5b6015
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 messageThis 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
-r
in fish autocomplete https://github.com/tldr-pages/tldr-c-client/commit/6cfe15a1b295725d726103c42507e7103acfd902tldr
accepts a-r
/--render
argument, which is used liketldr --render <PATH>
ortldr -r <PATH>
.The current behavior for fish autocompletion of the
--render
option addedPATH
as a literal value for completion, meaning that it would suggestPATH
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 for
complete
(fish builtin)Make fish-shell autocompletion smarter https://github.com/tldr-pages/tldr-c-client/commit/a839565e4b43fcb8e4768fd57c6b52830f11e736
tldr
fish autocompletion is now functional, but the autocompletion will sometimes suggest things that are invalid, such as:--update
,--help
, and--render
at the same time.--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".
Remove redundant options in fish completion https://github.com/tldr-pages/tldr-c-client/commit/2dc6b419df75d738f37c60f9895534dc9243ce2c
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.
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?
https://github.com/tldr-pages/tldr/pull/5205, https://github.com/tldr-pages/tldr/pull/7530, https://github.com/tldr-pages/tldr-c-client/pull/17, https://github.com/tldr-pages/tldr-c-client/issues/87
These changes are explicitly licensed under the MIT License.