completion: improve detection for flags that accept multiple values #2210
Conversation
thaJeztah
force-pushed
the
completion_improvements
branch
from
1e5ce18 to
9548b64
Compare
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
force-pushed
the
completion_improvements
branch
from
9548b64 to
8c1b26b
Compare
completions.go
Outdated
| // 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 { |
There was a problem hiding this comment.
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.
|
I pushed a commit addressing the comment, and doing a slight refactoring. |
There was a problem hiding this comment.
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
|
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 😅 |
|
@marckhouzam are you a maintainer for |
|
No, I’m not a maintainer for pflag |
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.