-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix CompletionFunc implementation #2234
Conversation
The new type CompletionFunc could lead to a regression. This commit adds a test to ensure that the completion function is correctly The tests are currently failing. This is expected as the fix is not yet implemented. Signed-off-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Signed-off-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
I provided a separate commit so anyone reviewing could fetch and check. I covered the possible issue such as type UserCompletionTypeHelper func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective)
type UserCompletionTypeAliasHelper = func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) Please review @marckhouzam @thaJeztah @tassa-yoniso-manasi-karoto Thanks |
I'm reviewing this now. Thanks for the quick fix @ccoVeille ! |
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.
LGTM, thank you!
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.
Very nice tests!
LGTM
Thanks!
Using the branch from spf13/cobra#2234 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Tested locally and build works with this PR (failed with v1.9.0); just pushed a draft PR to also have it run in CI docker/cli#5829 |
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.
Late comment: LGTM
This PR is about this