completion: improve detection for flags that accept multiple values #2210

thaJeztah · 2024-12-28T14:13:40Z

The completion code attempts to detect whether a flag can be specified more than once, and therefore should provide completion even if already set.

Currently, this code depends on conventions used in the pflag package, which uses an "Array" or "Slice" suffix or for some types a "stringTo" prefix.

Cobra allows custom value types to be used, which may not use the same convention for naming, and therefore currently aren't detected to allow multiple values.

The pflag module defines a SliceValue interface, which is implemented by the Slice and Array value types it provides (unfortunately, it's not currently implemented by the "stringTo" values).

This patch adds a reduced interface based on the SliceValue interface mentioned above to allow detecting Value-types that accept multiple values. Custom types can implement this interface to make completion work for those values.

I deliberately used a reduced interface to keep the requirements for this detection as low as possible, without enforcing the other methods defined in the interface (Append, Replace) which may not apply to all custom types.

Future improvements can likely still be made, considering either implementing the SliceValue interface for the "stringTo" values or defining a separate "MapValue" interface for those types.

Possibly providing the reduced interface as part of the pflag module and to export it.

The completion code attempts to detect whether a flag can be specified more than once, and therefore should provide completion even if already set. Currently, this code depends on conventions used in the pflag package, which uses an "Array" or "Slice" suffix or for some types a "stringTo" prefix. Cobra allows custom value types to be used, which may not use the same convention for naming, and therefore currently aren't detected to allow multiple values. The pflag module defines a [SliceValue] interface, which is implemented by the Slice and Array value types it provides (unfortunately, it's not currently implemented by the "stringTo" values). This patch adds a reduced interface based on the [SliceValue] interface mentioned above to allow detecting Value-types that accept multiple values. Custom types can implement this interface to make completion work for those values. I deliberately used a reduced interface to keep the requirements for this detection as low as possible, without enforcing the other methods defined in the interface (Append, Replace) which may not apply to all custom types. Future improvements can likely still be made, considering either implementing the SliceValue interface for the "stringTo" values or defining a separate "MapValue" interface for those types. Possibly providing the reduced interface as part of the pflag module and to export it. [SliceValue]: https://github.com/spf13/pflag/blob/d5e0c0615acee7028e1e2740a11102313be88de1/flag.go#L193-L203 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam · 2024-12-28T22:17:07Z

completions.go

+// sliceValue is a reduced version of [pflag.SliceValue]. It is used to detect
+// flags that accept multiple values and therefore can provide completion
+// multiple times.
+type sliceValue interface {


I think that if we want to allow programs to do
var _ sliceValue = (*customMultiString)(nil) like in the tests, we need to export the type.

marckhouzam · 2024-12-28T22:39:30Z

I pushed a commit addressing the comment, and doing a slight refactoring.

marckhouzam

Very nice!
@thaJeztah If you have comments on the commit I pushed on top of yours, please let me know and we can address in a follow-up

thaJeztah · 2024-12-30T13:59:32Z

Thanks! Changes make sense; I wasn't sure if we wanted a more solid / generic approach (possibly a dedicated interface or perhaps annotations for flags), hence I initially chose not to export the interface to keep some wiggle-room 😅

thaJeztah · 2024-12-30T14:03:08Z

@marckhouzam are you a maintainer for spf13/pflag as well? I recently noticed that some of our dependencies started to force an untagged version of the package (current master) through go modules; was wondering if a patch / minor release is planned so that we can go back to using a tagged release in those codebases; spf13/pflag@v1.0.5...d5e0c06

marckhouzam · 2024-12-30T17:17:00Z

No, I’m not a maintainer for pflag

ccoVeille · 2025-01-09T20:40:19Z

completions.go

+// SliceValue is a reduced version of [pflag.SliceValue]. It is used to detect
+// flags that accept multiple values and therefore can provide completion
+// multiple times.
+type SliceValue interface {
+	// GetSlice returns the flag value list as an array of strings.
+	GetSlice() []string
+}


Can you do something like this either here or in the tests?

(This is untested pseudocode written on a phone, please check and adapt)

Suggested change

// SliceValue is a reduced version of [pflag.SliceValue]. It is used to detect

// flags that accept multiple values and therefore can provide completion

// multiple times.

type SliceValue interface {

// GetSlice returns the flag value list as an array of strings.

GetSlice() []string

}

// SliceValue is a reduced version of [pflag.SliceValue]. It is used to detect

// flags that accept multiple values and therefore can provide completion

// multiple times.

type SliceValue interface {

// GetSlice returns the flag value list as an array of strings.

GetSlice() []string

}

var _ SliceValue = (*pflag.SliceValue)(nil)

The idea is to validate pflag.SliceValue implements the interface you define

@thaJeztah I'm unsure if you saw my comment last week

ccoVeille · 2025-01-09T20:44:47Z

completions_test.go

+var _ SliceValue = (*customMultiString)(nil)
+
+func (s *customMultiString) String() string {
+	return fmt.Sprintf("%v", *s)


What about this?

Suggested change

return fmt.Sprintf("%v", *s)

return fmt.Sprint(*s)

Cobra's shell completion has specific rules to decide whether a flag can be accepted multiple times. If a flag does not meet that rule, it only completes the flag name once; some of those rules depend on the "type" of the option to end with "Array" or "Slice", which most of our options don't. Starting with Cobra 1.9, it also checks whether an option implements the [cobra.SliceValue] interface (see [spf13/cobra 2210]). This patch implements the [cobra.SliceValue] interface on ListOpts, so that these options can be completed multiple times. In a follow-up, we can update our code to replace our uses of `GetAll()`, which is identical with the `GetSlice()` method, and potentially deprecate the old method. Before this patch, ListOpts would only be completed once when completing flag names. For example, the following would show the `--label` flag the first time, but omit it if a `--label` flag was already set; docker run--l<TAB> --label (Set meta data on a container) --link-local-ip (Container IPv4/IPv6 link-local addresses) --label-file (Read in a line delimited file of labels) --log-driver (Logging driver for the container) --link (Add link to another container) --log-opt (Log driver options) docker run --label hello --l<TAB> --label-file (Read in a line delimited file of labels) --link-local-ip (Container IPv4/IPv6 link-local addresses) --log-opt (Log driver options) --link (Add link to another container) --log-driver (Logging driver for the container) With this patch, the completion script correctly identifies the `--label` flag to be accepted multiple times, and also completes it when already set; docker run --label hello --l<TAB> --label (Set meta data on a container) --link-local-ip (Container IPv4/IPv6 link-local addresses) --label-file (Read in a line delimited file of labels) --log-driver (Logging driver for the container) --link (Add link to another container) --log-opt (Log driver options) [cobra.SliceValue]: https://pkg.go.dev/github.com/spf13/cobra@v1.9.1#SliceValue [spf13/cobra 2210]: spf13/cobra#2210 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/spf13/cobra](https://github.com/spf13/cobra) | require | minor | `v1.8.1` -> `v1.9.1` | --- ### Release Notes <details> <summary>spf13/cobra (github.com/spf13/cobra)</summary> ### [`v1.9.1`](https://github.com/spf13/cobra/releases/tag/v1.9.1) [Compare Source](spf13/cobra@v1.9.0...v1.9.1) ##### 🐛 Fixes - Fix CompletionFunc implementation by [@ccoVeille](https://github.com/ccoVeille) in spf13/cobra#2234 - Revert "Make detection for test-binary more universal ([#2173](spf13/cobra#2173))" by [@marckhouzam](https://github.com/marckhouzam) in spf13/cobra#2235 **Full Changelog**: spf13/cobra@v1.9.0...v1.9.1 ### [`v1.9.0`](https://github.com/spf13/cobra/releases/tag/v1.9.0) [Compare Source](spf13/cobra@v1.8.1...v1.9.0) #### ✨ Features - Allow linker to perform deadcode elimination for program using Cobra by [@aarzilli](https://github.com/aarzilli) in spf13/cobra#1956 - Add default completion command even if there are no other sub-commands by [@marckhouzam](https://github.com/marckhouzam) in spf13/cobra#1559 - Add CompletionWithDesc helper by [@ccoVeille](https://github.com/ccoVeille) in spf13/cobra#2231 #### 🐛 Fixes - Fix deprecation comment for Command.SetOutput by [@thaJeztah](https://github.com/thaJeztah) in spf13/cobra#2172 - Replace deprecated ioutil usage by [@nirs](https://github.com/nirs) in spf13/cobra#2181 - Fix --version help and output for plugins by [@nirs](https://github.com/nirs) in spf13/cobra#2180 - Allow to reset the templates to the default by [@marckhouzam](https://github.com/marckhouzam) in spf13/cobra#2229 #### 🤖 Completions - Make Powershell completion work in constrained mode by [@lstemplinger](https://github.com/lstemplinger) in spf13/cobra#2196 - Improve detection for flags that accept multiple values by [@thaJeztah](https://github.com/thaJeztah) in spf13/cobra#2210 - add CompletionFunc type to help with completions by [@ccoVeille](https://github.com/ccoVeille) in spf13/cobra#2220 - Add similar whitespace escape logic to bash v2 completions than in other completions by [@kangasta](https://github.com/kangasta) in spf13/cobra#1743 - Print ActiveHelp for bash along other completions by [@marckhouzam](https://github.com/marckhouzam) in spf13/cobra#2076 - fix(completions): Complete map flags multiple times by [@gabe565](https://github.com/gabe565) in spf13/cobra#2174 - fix(bash): nounset unbound file filter variable on empty extension by [@scop](https://github.com/scop) in spf13/cobra#2228 #### 🧪 Testing - Test also with go 1.23 by [@nirs](https://github.com/nirs) in spf13/cobra#2182 - Make detection for test-binary more universal by [@thaJeztah](https://github.com/thaJeztah) in spf13/cobra#2173 #### ✍🏼 Documentation - docs: update README.md by [@eltociear](https://github.com/eltociear) in spf13/cobra#2197 - Improve site formatting by [@nirs](https://github.com/nirs) in spf13/cobra#2183 - doc: add Conduit by [@raulb](https://github.com/raulb) in spf13/cobra#2230 - doc: azion project added to the list of CLIs that use cobra by [@maxwelbm](https://github.com/maxwelbm) in spf13/cobra#2198 - Fix broken links in active_help.md by [@vuil](https://github.com/vuil) in spf13/cobra#2202 - chore: fix function name in comment by [@zhuhaicity](https://github.com/zhuhaicity) in spf13/cobra#2216 #### 🔧 Dependency upgrades - build(deps): bump github.com/cpuguy83/go-md2man/v2 from 2.0.5 to 2.0.6 by [@thaJeztah](https://github.com/thaJeztah) in spf13/cobra#2206 - Update to latest go-md2man by [@mikelolasagasti](https://github.com/mikelolasagasti) in spf13/cobra#2201 - Upgrade `pflag` dependencies for v1.9.0 by [@jpmcb](https://github.com/jpmcb) in spf13/cobra#2233 *** Thank you to all of our amazing contributors and all the great work that's been going into the completions feature!! ##### 👋🏼 New Contributors - [@gabe565](https://github.com/gabe565) made their first contribution in spf13/cobra#2174 - [@maxwelbm](https://github.com/maxwelbm) made their first contribution in spf13/cobra#2198 - [@lstemplinger](https://github.com/lstemplinger) made their first contribution in spf13/cobra#2196 - [@vuil](https://github.com/vuil) made their first contribution in spf13/cobra#2202 - [@mikelolasagasti](https://github.com/mikelolasagasti) made their first contribution in spf13/cobra#2201 - [@zhuhaicity](https://github.com/zhuhaicity) made their first contribution in spf13/cobra#2216 - [@ccoVeille](https://github.com/ccoVeille) made their first contribution in spf13/cobra#2220 - [@kangasta](https://github.com/kangasta) made their first contribution in spf13/cobra#1743 - [@aarzilli](https://github.com/aarzilli) made their first contribution in spf13/cobra#1956 **Full Changelog**: spf13/cobra@v1.8.1...v1.9.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/557 Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org> Co-authored-by: Renovate Bot <bot@kriese.eu> Co-committed-by: Renovate Bot <bot@kriese.eu>

thaJeztah mentioned this pull request Dec 28, 2024

Feature request: Set a default completion function #2209

Closed

thaJeztah force-pushed the completion_improvements branch from 1e5ce18 to 9548b64 Compare December 28, 2024 14:15

thaJeztah and others added 2 commits December 28, 2024 17:07

Address comments

8c1b26b

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam force-pushed the completion_improvements branch from 9548b64 to 8c1b26b Compare December 28, 2024 22:38

marckhouzam reviewed Dec 28, 2024

View reviewed changes

marckhouzam approved these changes Dec 28, 2024

View reviewed changes

marckhouzam merged commit 0745e55 into spf13:main Dec 28, 2024
20 checks passed

marckhouzam added this to the 1.9.0 milestone Dec 28, 2024

thaJeztah deleted the completion_improvements branch December 30, 2024 13:55

ccoVeille reviewed Jan 9, 2025

View reviewed changes

renovate bot mentioned this pull request Feb 28, 2025

Update grouped non-major updates aenix-io/etcd-operator#237

Open

1 task

thaJeztah mentioned this pull request Apr 23, 2025

opts: ListOpts: implement cobra.SliceValue to fix shell completion docker/cli#6030

Merged

yashuatla mentioned this pull request Jun 25, 2025

Feature implementation from commits 78bfc83..41b26ec yashuatla/cobra#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

completion: improve detection for flags that accept multiple values #2210

completion: improve detection for flags that accept multiple values #2210

Uh oh!

thaJeztah commented Dec 28, 2024

Uh oh!

marckhouzam Dec 28, 2024

Uh oh!

marckhouzam commented Dec 28, 2024

Uh oh!

marckhouzam left a comment

Uh oh!

Uh oh!

thaJeztah commented Dec 30, 2024

Uh oh!

thaJeztah commented Dec 30, 2024

Uh oh!

marckhouzam commented Dec 30, 2024

Uh oh!

ccoVeille Jan 9, 2025

Uh oh!

ccoVeille Jan 15, 2025

Uh oh!

ccoVeille Jan 9, 2025

Uh oh!

ccoVeille Jan 15, 2025

Uh oh!

Uh oh!

completion: improve detection for flags that accept multiple values #2210

completion: improve detection for flags that accept multiple values #2210

Uh oh!

Conversation

thaJeztah commented Dec 28, 2024

Uh oh!

marckhouzam Dec 28, 2024

Choose a reason for hiding this comment

Uh oh!

marckhouzam commented Dec 28, 2024

Uh oh!

marckhouzam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Dec 30, 2024

Uh oh!

thaJeztah commented Dec 30, 2024

Uh oh!

marckhouzam commented Dec 30, 2024

Uh oh!

ccoVeille Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

ccoVeille Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!