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

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
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Collaborator

@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
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice! Each mode works great!

@Luap99
Copy link
Contributor Author

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
Collaborator

@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
Collaborator

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 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
Collaborator

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 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
Copy link
Contributor Author

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
Collaborator

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
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 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
Collaborator

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 Compare September 7, 2020 13:06
@Luap99
Copy link
Contributor Author

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 September 7, 2020 13:22
Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Collaborator

@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 added the area/flags-args Changes to functionality around command line flags and args label Dec 18, 2020
@jpmcb jpmcb modified the milestones: 2.0.0, Next Dec 18, 2020
@jpmcb jpmcb removed the area/flags-args Changes to functionality around command line flags and args label Dec 18, 2020
@jharshman jharshman modified the milestones: 2.0.0, Next Dec 20, 2020
Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] When completion Item description is empty, Powershell completion is not working
6 participants