Skip to content
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

powershell completion with custom comp #1208

Merged
merged 1 commit into from Dec 29, 2020
Merged

Conversation

@Luap99
Copy link
Contributor

@Luap99 Luap99 commented Aug 26, 2020

The current powershell completion is not very capable.

Let's port it to the go custom completion logic to have a
unified experience accross all shells.

Powershell supports three different completion modes

  • TabCompleteNext (default windows style - on each key press the next option is displayed)
  • Complete (works like bash)
  • MenuComplete (works like zsh)

You set the mode with Set-PSReadLineKeyHandler -Key Tab -Function <mode>

Descriptions will only be supported for Complete and MenuComplete.

Fixes #1229

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Thanks for doing this!
I haven't looked in detail but just a quick comment.

powershell_completions.go Outdated Show resolved Hide resolved
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Aug 26, 2020

@marckhouzam I don't think I can get ShellCompDirectiveFilterFileExt and ShellCompDirectiveFilterDirs to work, I guess the best way is to provide the standard path completion instead?

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Aug 26, 2020

@Luap99 agreed. That's what I also had to do for fish completion:

cobra/fish_completions.go

Lines 126 to 133 in 02a0d2f

set filefilter (math (math --scale 0 $directive / $shellCompDirectiveFilterFileExt) %% 2)
set dirfilter (math (math --scale 0 $directive / $shellCompDirectiveFilterDirs) %% 2)
if test $filefilter -eq 1; or test $dirfilter -eq 1
__%[1]s_debug "File extension filtering or directory filtering not supported"
# Do full file completion instead
set --global __%[1]s_comp_do_file_comp 1
return 1
end

Copy link
Contributor

@marckhouzam marckhouzam left a comment

@Luap99 this works amazingly well!
I've pointed helm to use your PR and it looks really great!
I've also done similar testing as I had done when working on the zsh v2 script, so it covers pretty much everything I think.

I just have some minor comments.
Great job!

powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
# - TabCompleteNext (default windows style - on each key press the next option is displayed)
# - Complete (works like bash)
# - MenuComplete (works like zsh)
# You set the mode with Set-PSReadLineKeyHandler -Key Tab -Function <mode>
Copy link
Contributor

@marckhouzam marckhouzam Aug 27, 2020

Super nice! Each mode works great!

@Luap99 Luap99 force-pushed the powershell-custom-comp branch from 62508d9 to d7aac2f Aug 27, 2020
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Aug 27, 2020

@marckhouzam Thanks for the fast review. I ignore ShellCompDirectiveFilterFileExt and ShellCompDirectiveFilterDirs now because there to many corner cases I can't handle.

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Aug 28, 2020

@Luap99 Everything works great! As you said, the --flag= form would be nice, but personally, I don't think it should be a blocker. The documentation is important but after that, I would love to see this merged.

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Aug 28, 2020

Oh, and I think you should remove powershell_completions_test.go.
I'm not sure if there are tests you can implement since there is no more generation of code besides the fixed script.
I removed zsh_completions_test.go when I did the zsh port to Go completions.

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Aug 28, 2020

Luap99 Everything works great! As you said, the --flag= form would be nice, but personally, I don't think it should be a blocker. The documentation is important but after that, I would love to see this merged.

I'm still thinking how this could be done. Powershell does not have COMP_WORDBREAKS, so I can only imagine something like this.

$ ./testprog --flag=[TAB]
--flag=foo
--flag=bar
--flag=foobar

Not very nice but it could work.

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Aug 28, 2020

I'm still thinking how this could be done. Powershell does not have COMP_WORDBREAKS, so I can only imagine something like this.

$ ./testprog --flag=[TAB]
--flag=foo
--flag=bar
--flag=foobar

Not very nice but it could work.

This is what helm and kubectl currently offer for their zsh completion (not yet migrated to the new Cobra zsh v2):

kubectl --context=<TAB>
--context=acc       --context=cor       --context=dev       --context=lab       --context=minikube
--context=bi.dev    --context=default   --context=infra     --context=lab2      --context=prod

I think it is just fine.

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Aug 28, 2020

This is what helm and kubectl currently offer for their zsh completion (not yet migrated to the new Cobra zsh v2):

I'm curios does that work with file path completion?

I got it working for powershell with arguments, but there is no way to make it work with file completion.

@Luap99 Luap99 force-pushed the powershell-custom-comp branch from d7aac2f to 4af1e92 Aug 28, 2020
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Aug 28, 2020

Ok, I think the script is in good shape now. I have added documentation, but it still needs to be refined.

One question remains, how should the API look like?

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Aug 29, 2020

This is what helm and kubectl currently offer for their zsh completion (not yet migrated to the new Cobra zsh v2):

I'm curious does that work with file path completion?

I hadn't realized, but no, it does not work with file completion. So, currently, for zsh (not using Cobra's zsh support):

$ kubectl --kubeconfig /User<TAB>     # Works
$ kubectl --kubeconfig=/User<TAB>     # Does not work

I got it working for powershell with arguments, but there is no way to make it work with file completion.

I think that is fine.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Except for the API backwards-compatibility (which can be fixed easily), this looks ready and of great quality.

@jpmcb You may be interested in trying this out.

@jharshman In my opinion, it would be good to include this in the coming release. With this PR, Cobra provides full auto-completion for all 4 shells it supports.

shell_completions.md Outdated Show resolved Hide resolved
shell_completions.md Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
powershell_completions.go Outdated Show resolved Hide resolved
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Aug 29, 2020

I just tested this on windows 10 with the default powershell v5.1 and I noticed one problem. testprog.exe --[TAB] doesn't work but I think this is not a bug in my script since Powershell doesn't call the completion script at all (there is no debug output). Also testprog.exe `--[TAB] and testprog.exe --r[TAB] are working. Since testprog.exe --[TAB] works in Powershell v7, I call this a bug in Powershell v5.1.
I think there is nothing I can do other then adding a note in the docs.

Apart from that, I think it works very well.

@Luap99 Luap99 changed the title WIP: powershell completion with custom comp powershell completion with custom comp Aug 29, 2020
@jpmcb
Copy link
Collaborator

@jpmcb jpmcb commented Aug 31, 2020

I have no way of testing this on a windows machine. Are there other's in the community that would be able to lend a hand and test this out?

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Sep 6, 2020

I just tested this on windows 10 with the default powershell v5.1 and I noticed one problem. testprog.exe --[TAB] doesn't work but I think this is not a bug in my script since Powershell doesn't call the completion script at all (there is no debug output). Also testprog.exe `--[TAB] and testprog.exe --r[TAB] are working. Since testprog.exe --[TAB] works in Powershell v7, I call this a bug in Powershell v5.1.
I think there is nothing I can do other then adding a note in the docs.

I can confirm I see this same behavior with powershell 5.1. I think we'll just have to live with it.

I've tested on Mac with pwsh 7.0.2 and on Windows 10 with powershell 5.1 by using this PR with helm.
Everything works really well.
Beyond standard completions, I've tested:

  • ShellDirectiveNoSpace
  • ShellDirectiveNoFileComp
  • flag completion with the = form and the normal form
  • descriptions
  • all three types of completions for powershell

From a functionality perspective, not only do I feel this is complete, but I believe it is dramatically better than what we have right now for powershell.

@jpmcb There is still a need for guidance on what form the API should take for programs to call this.

@Luap99 Luap99 force-pushed the powershell-custom-comp branch 2 times, most recently from d6075ef to ccb09b7 Sep 7, 2020
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Sep 7, 2020

@marckhouzam Thanks for testing.

I added one small thing to properly escape spacial chars with a backtick. So they can safely be used as input.

I also changed the API to GenPowerShellCompletion() and GenPowerShellCompletionWithDesc() to keep it backwards compatible.

@Luap99 Luap99 marked this pull request as ready for review Sep 7, 2020
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Very nice.
Could you just add powershell in shell_completions.md in the Descriptions for completions section?

The current powershell completion is not very capable.

Let's port it to the go custom completion logic to have a
unified experience accross all shells.

Powershell supports three different completion modes

- TabCompleteNext (default windows style - on each key press the next option is displayed)
- Complete (works like bash)
- MenuComplete (works like zsh)

You set the mode with `Set-PSReadLineKeyHandler -Key Tab -Function <mode>`

To keep it backwards compatible `GenPowerShellCompletion` will not display descriptions.
Use `GenPowerShellCompletionWithDesc` instead. Descriptions will only be displayed with
`MenuComplete` or `Complete`.

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Nov 21, 2020

@jpmcb what do you think about merging this powershell completion patch? From my testing @Luap99 did a spectacular job with this.

Furthermore, it seems no one can get the existing powershell completion to work at all currently, so there is no risk of breaking anything 😄 In fact, I checked-out the commit from the original powershell completion pr #797 and still only got flag completion to work; it seems the feature never properly worked.

We need some good material for the next release and I thought this would be a great one.

@jpmcb jpmcb removed this from the 2.0.0 milestone Dec 18, 2020
@jpmcb jpmcb added this to the Next milestone Dec 18, 2020
@jpmcb jpmcb removed this from the Next milestone Dec 18, 2020
@jpmcb jpmcb added this to the 2.0.0 milestone Dec 18, 2020
@jpmcb jpmcb removed the area/flags label Dec 18, 2020
@jharshman jharshman removed this from the 2.0.0 milestone Dec 20, 2020
@jharshman jharshman added this to the Next milestone Dec 20, 2020
jpmcb
jpmcb approved these changes Dec 29, 2020
Copy link
Collaborator

@jpmcb jpmcb left a comment

Thanks for the time and patience on this one - I'll go ahead and merge since there seems to be a consensus that this is indeed working on windows / powershell.

If anyone in the community is trying this out and finds issues, please feel free to open a new issue!!

@jpmcb jpmcb merged commit a4ab3fa into spf13:master Dec 29, 2020
2 checks passed
@Luap99 Luap99 deleted the powershell-custom-comp branch Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants