feat: add CompletionWithDesc helper

[ccoVeille]

The code has also been refactored to use a type alias for completion and a completion helper

Using a type alias is a non-breaking change, and it makes the code more readable and easier to understand.

Fixes #2222

[marckhouzam]

My only hesitation about making this change is that programs that want to use the new type alias will need to replace []string with []cobra.CompletionChoice which is long.

Would cobra.Completion or cobra.CompChoice or even cobra.Comp be sufficiently clear or does it become too cryptic?

[ccoVeille]

The type alias is not required. But I get your point. IDE or AI tool will complete it this way

I think cobra.Completion is clear enough.

But then, we also move to cobra.CompletionWithDescription ?

I'm fine with CompletionWithDescription and CompletionWithDesc

What would be your choice?

[marckhouzam]

OK for cobra.Completion
As for CompletionWithDescription, since that’s a function, I’m not as concerned. But I’ve used “Desc” multiple times before in cobra, so let’s go with “CompletionWithDesc”

[marckhouzam]

I tried this with tanzu and I like the way it looks; it makes the code much clearer.
Thanks for doing this!

I went ahead and used your new Completion and CompletionWithDesc in the rest of the Cobra code base. If you provide me with permissions, I'll push a commit on top of yours; this will be much easier for me than to provide 30 different comments.

[marckhouzam]

I've just pushed a commit to use the new type alias and function everywhere.
@ccoVeille Can you review and let me know if you agree?
If you don't find anything else, we can merge this.

[ccoVeille]

I'm more than fine with the changes, you did.

I love the way how it somehow shows the changed was not only a good thing, but needed.

[marckhouzam]

Very nice!
Thanks!

[marckhouzam]

From what I can see Go can handle the conversion between the original func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) and the new cobra.CompletionFunc.

The same goes for the type defined by the docker CLI:
type ValidArgsFn func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective)

The problem happens because now we are asking go to accept the type ValidArgsFn (of the docker CLI) as the type cobra.CompletionFunc, which, although they are the same, is understandably not a valid conversion.

Cobra should not break programs, so we should fix this in Cobra; who knows how many other programs use their own type for the completion functions.

We could simply revert and not define cobra.CompletionFunc.
Alternatively, I think that making this new type a type alias would also work:
type CompletionFunc = func(cmd *Command, args []string, toComplete string) ([]Completion, ShellCompDirective)
Notice the extra = in the definition.

What do people think? I haven't used type aliases much so I don't have a good feel for it.
@jpmcb @thaJeztah @ccoVeille

[ccoVeille]

Thank you @marckhouzam for the investigation

Sorry for the inconvenience @thaJeztah or anyone who may have face this issue.

I thibk the issue is in fact related to #2220 where CompletionFunc was added

• feat: add CompletionFunc type to help with completions #2220

The changes in the current PR and #2220 were released at same time.

I think the solution of using a type alias for CompletionFunc so adding an alias is a good one.

I'm surprised the problem was not covered by a test, I can add one.

I would recommend to retract 1.9.0 for now via a 1.9.1

But the fix seems easy. So let's try to fix it

[thaJeztah]

Thanks both! Sorry for my rather brief comment earlier; I saw the issue reported and only had time to dive in for a tiny bit because I had to head out.

I'm not sure how widely spread the problem is; if the issue is purely in our specific situation or more widespread. At least I saw that part of the issue on our side is that we define a ValidArgsFn type, which was matching the cobra signature. To be honest, I'm not even sure why we define that type; I have yet to dive in git history to see if there's more context, but it could've been just for convenience, and in that case more likely should've been an alias (and documented better on the reason it's there).

I suspect that fixing on our side would not be too complicated and could possibly be limited to just that (aliasing our ValidArgsFn to cobra's type) but I haven't tried yet. That would not be immediately helping users who want to update Cobra though, as we'd have to get release(s) out the door.

Looking at the comment, I see it was mostly done to create a "pseudo" type to make for a nicer documentation;

Note: Go type alias is used to provide a more descriptive name in the documentation, but any string can be used.

I've done similar tricks in some cases for the same reason, and most of the time it works without issues, but there's definitely been cases where it's slightly more involved or not possible without breaking things (unfortunately!).

So .. possibly changing back the Completion to a regular string would be the safest option, at the cost of slightly poorer documentation. Perhaps a middle ground could be to add named output vars to the CompletionFunc type, something like;

type CompletionFunc func(cmd *Command, args []string, toComplete string) (completion []string, _ ShellCompDirective)

thibk the issue is in fact related to #2220 where CompletionFunc was added

Ah, possibly that sounds plausible as well, and possibly could also explain why we have that local ValidArgsFn type defined (again, I yet have to dive into history, I just got back home).

[thaJeztah]

I did a quick test, and it seems that making the ValidArgsFn on our side an alias, so not a distinct type, works; even when the signature keeps []string. We may be making that change in either case (or change it to an alias for CompletionFunc), but I don't have an ETA yet on our side for a potential (patch) release, and of course it may be best to go for safe and also make changes in Cobra;

• [27.x] cli/command/completion: alias ValidArgsFn instead of strong type docker/cli#5828

(I asked @tassa-yoniso-manasi-karoto, who reported the issue (thanks!!) to verify if that approach works)

[ccoVeille]

Thanks for your note @thaJeztah

I managed to understand the issue. I confirm adding the type alias as @marckhouzam suggested fixes the issue.

I managed to replicate the issue locally.

I'm working on a fix.

[thaJeztah]

Awesome, thank you! And no stress (please!) the world didn't burn because of this, and it was great to be discovered this soon after the release.

[ccoVeille]

Here is the fix

• fix CompletionFunc implementation #2234