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

Custom completion handle multiple shorhand flags together #1258

Merged
merged 1 commit into from May 3, 2021

Conversation

@Luap99
Copy link
Contributor

@Luap99 Luap99 commented Oct 15, 2020

Flag definitions like -asd are not handled correctly by
the custom completion logic. They should be treated as
multiple flags. For details refer to #1257.

Fixes #1257

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Oct 15, 2020

Thanks @Luap99, this looks great. The scenario with = does not work perfectly, but it's not critical.

Say I have 2 boolean flags -b and a string flag -s with completions string1 and string2.

./testprog -bs=<TAB> won't complete properly.

If fixing that makes the code much more complicated, we may want to ignore that case

@Luap99 Luap99 force-pushed the Luap99:fix-1257 branch from df56fef to 6746e67 Oct 15, 2020
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Oct 15, 2020

@marckhouzam Thanks, this should work now.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Looks good! Just some minor nits.

}

// Test that multiple boolean + string with equal sign with value shorthand flags work
output, err = executeCommand(rootCmd, ShellCompNoDescRequestCmd, "-sdf=abc", "")

This comment has been minimized.

@marckhouzam

marckhouzam Oct 15, 2020
Contributor

Could you add one more test:
output, err = executeCommand(rootCmd, ShellCompNoDescRequestCmd, "-sdf", "works", "")

This comment has been minimized.

@Luap99

Luap99 Oct 15, 2020
Author Contributor

This does not work. 😢 I think this is a internal cobra problem since the error comes from c.Root().Find(trimmedArgs)

This comment has been minimized.

@marckhouzam

marckhouzam Nov 23, 2020
Contributor

I believe you found a small bug in Cobra. The stripFlags() function does not handle multiple short flags properly.

This comment has been minimized.

@marckhouzam

marckhouzam Nov 23, 2020
Contributor

It may become important to fix this as it sounds like it would affect some completion scenarios

This comment has been minimized.

@jpmcb

jpmcb May 3, 2021
Collaborator

I'm assuming fixing stripFlags() is out of scope for this PR?

We may want a separate issue to track this and get it fixed, but if it's going to block functionality for shorthand flags working, we may want to tackle it now. Thoughts?

This comment has been minimized.

@Luap99

Luap99 May 3, 2021
Author Contributor

This is definitely out of scope for this PR. This PR fixes a very annoying issue, I would love to have this merged.
There is already this issue #618.

This comment has been minimized.

@jpmcb

jpmcb May 3, 2021
Collaborator

Awesome - I'll approve and merge!

custom_completions.go Outdated Show resolved Hide resolved
custom_completions.go Outdated Show resolved Hide resolved
@Luap99 Luap99 force-pushed the Luap99:fix-1257 branch 2 times, most recently from d94da2d to 1918d5e Oct 15, 2020
@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Nov 23, 2020

I've just hit this bug using kubectl.

I'm trying out Go completions and native zsh completion in kubectl to make sure the PR moving kubectl to go completions is good: kubernetes/kubernetes#96087

For the kubectl exec command, I always use the form kubectl exec -it instead of kubectl exec -i -t, which triggers this bug.

With this in mind, it would be nice to add this PR to the next Cobra release. Would a maintainer be so kind as to add it to the 2.0.0 milestone? Thank you.

@vrothberg
Copy link

@vrothberg vrothberg commented Dec 1, 2020

@eparis, could you take a look? Podman needs this patch and we don't want to vendor a fork too long. Downstream packaging is impacted by it.

Great work, @Luap99 !

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Feb 14, 2021

@jpmcb This small fix would be nice to include as it causes a regression for bash V2 (#1146) compared to bash V1

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Feb 14, 2021

@jpmcb This small fix would be nice to include as it causes a regression for bash V2 (#1146) compared to bash V1

Not only bash v2 it also affects zsh and fish which are already used.

@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Feb 15, 2021

Not only bash v2 it also affects zsh and fish which are already used.

In fact, I now realize it affects all Go completions (ValidArgsFunction and RegisterFlagCompletionFunc()), including bash v1. This will cause a regression for kubectl when it moves to Go completions (see above comment #1258 (comment)) /cc @jpmcb

Flag definitions like `-asd` are not handled correctly by
the custom completion logic. They should be treated as
multiple flags. For details refer to #1257.

Fixes #1257

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@Luap99 Luap99 force-pushed the Luap99:fix-1257 branch from 1918d5e to 32a703e Feb 18, 2021
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Feb 18, 2021

I just rebased to resolve the merge conflict.

@jpmcb
jpmcb approved these changes May 3, 2021
Copy link
Collaborator

@jpmcb jpmcb left a comment

Thanks for this! LGTM

We also have #618 to track the found issue around the stripFlags function. We'll need to tackle that outside this PR.

@jpmcb jpmcb merged commit 2d94892 into spf13:master May 3, 2021
8 checks passed
8 checks passed
@github-actions
triage
Details
@github-actions
ubuntu | 1.14.x
Details
@github-actions
ubuntu | 1.15.x
Details
@github-actions
macOS | 1.14.x
Details
@github-actions
macOS | 1.15.x
Details
@github-actions
MINGW64
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@Luap99 Luap99 deleted the Luap99:fix-1257 branch May 3, 2021
@jpmcb jpmcb mentioned this pull request May 3, 2021
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.

5 participants