completion: improve detection for flags that accept multiple values #2210
+47
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
Signed-off-by: Marc Khouzam <[email protected]>
marckhouzam
force-pushed
the
completion_improvements
branch
from
9548b64 to
8c1b26b
Compare
marckhouzam
reviewed
Dec 28, 2024
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. |
marckhouzam
approved these changes
Dec 28, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.