-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Restrict completion to one suggestion on run, history, push, tag #5818
Restrict completion to one suggestion on run, history, push, tag #5818
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #5818 +/- ##
==========================================
- Coverage 59.28% 59.27% -0.01%
==========================================
Files 352 352
Lines 29496 29499 +3
==========================================
Hits 17487 17487
- Misses 11033 11035 +2
- Partials 976 977 +1 |
cli/command/completion/functions.go
Outdated
@@ -29,6 +29,12 @@ type APIClientProvider interface { | |||
// ImageNames offers completion for images present within the local store | |||
func ImageNames(dockerCLI APIClientProvider) ValidArgsFn { | |||
return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | |||
switch cmd.Name() { | |||
case "run", "history", "push", "tag": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wondering a bit on tag
. Overriding an existing image tag is also a valid behavior, should we allow it to have 2 image arguments?
cli/command/completion/functions.go
Outdated
func ImageNames(dockerCLI APIClientProvider) ValidArgsFn { | ||
return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | ||
switch cmd.Name() { | ||
case "run", "history", "push", "tag": | ||
if len(args) > 0 { | ||
return nil, cobra.ShellCompDirectiveNoFileComp | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should avoid hardcoding the commands in this function and make the caller responsible for picking the desired behavior.
We could add an argument to ImageNames
which would take a maximum number of completable images. IMO this could even be an enum with values of One | Two | Infinity
.
This would also work nicely with the "two image" case for tag
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, was considering the same. I think just passing an integer would work? (i.e., no magic const needed for this); after all, that's also what we do for number of arguments required (or min/maximum number for those, but minimum number doesn't really apply here);
Lines 49 to 50 in 516e822
// RequiresMaxArgs returns an error if there is not at most max args | |
func RequiresMaxArgs(maxArgs int) cobra.PositionalArgs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still commands like save
or inspect
, which can operate on multiple images, so my intent was to avoid having a "magic value" like -1 for that purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, yeah, not massively disturbed by that though, as it would match, e.g. strings.Replace()
and many other stdlib functions, which also use -1
(in this case 0
would even work). Alternative would be to have a separate function for "single" versus "multiple' images. (ImageName()
, ImageNames
with the latter one taking a limit as second argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative would be to have a separate function for "single" versus "multiple' images. (
ImageName()
,ImageNames
with the latter one taking a limit as second argument.
It doesn't solve this issue though, because you still have to distinguish "infinity" from a fixed amount 😅
But yeah, maybe -1 isn't that bad, considering that this pattern is also used in stdlib..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having a new parameter(say count) for ImageNames and -1 for infinity seems to cover all our requirements.
but then the validArgsFunction field only accepts the function of structure
func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective)
as a value. so we won't be able to pass in a new parameter to specify how many suggestions to stop on.
we could have different functions for 1, 2 and infinity tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry for brevity; commenting from my phone)
ImageNames
returns a closure, so it can capture variables passed into it. For example, this would capture the "limit" variable in the function that it returns, so now the function will only allow up to "limit" completions;
func ImageNames(dockerCLI APIClientProvider, limit int) ValidArgsFn {
return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
if limit > 0 && len(args) >= limit {
return nil, cobra.ShellCompDirectiveNoFileComp
}
// ...
Basically when calling the ImageNames
, it returns a custom function with specific options baked into it (in this case the dockerCLI
and limit
options)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it. thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM; One last thing - could you please squash commits into one?
…Names Previously, multiple suggestions were provided when completing commands like `run`, `history` and `push`. This change limits completion to a single suggestion for the above and 2 suggestions for `tag` Signed-off-by: Mohammed Aminu Futa <[email protected]>
5ca9994
to
a656dfd
Compare
Done squashing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #5814
Previously, multiple suggestions were provided when completing
commands like
run
,history
,push
, andtag
. This changelimits completion to a single suggestion for improved accuracy
and usability.
What I Did
ImageNames
function to returnnil
when the commandsrun
,history
,push
, andtag
have at least one suggestion.How I Did It
ImageNames
to prevent multiple suggestionsfrom being displayed when completion is triggered.
How to Verify It