-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
custom comp: do not complete flags after args when interspersed is false #1308
Conversation
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.
Nicely done!
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.
This is really nice.
I have mentioned in the comments two more problematic cases, but they are corner cases, especially the one about the args
being incomplete.
I don't know if supporting these cases is worth the possible extra complexity to the code, but I haven't tried to fix it myself, so I'm not sure. I personally would not consider them blockers as this PR already improves this scenario for the vast majority of cases.
Thanks for the continuing great work @Luap99 !
ab9afbc
to
9805c41
Compare
custom_completions_test.go
Outdated
childCmd.ValidArgsFunction = func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { | ||
return args, ShellCompDirectiveDefault | ||
} | ||
output, err = executeCommand(rootCmd, ShellCompRequestCmd, "child", "--", "--validarg", "") |
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.
We have to use a valid flag for this test, which will then trigger the problem:
output, err = executeCommand(rootCmd, ShellCompRequestCmd, "child", "--", "--string", "")
--validarg
is not found as a valid flag by checkIfFlagCompletion()
so your new logic handling errors is triggered. However, with --string
, there will not be an error as it is a valid flag and the --string
argument will be trimmed:
Line 466 in a4ab3fa
trimmedArgs = args[:len(args)-1] |
But this is such a rare (and possibly invalid) case that I don't think we should fix it (unless it is a very simple fix).
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.
You are right catching these cases is more difficult and would require a bigger refactor of the checkIfFlagCompletion
function.
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.
This looks good to me. It makes our completion support even more complete (no pun intended 😄 ).
I just rebased to solve the merge conflict. |
This PR is being marked as stale due to a long period of inactivity |
@jpmcb If you still have energy and time, this is another fix for completion which I had forgotten to mention. This will make the last Fish shell test of the |
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.
Hi @Luap99 - getting a number of these PRs in to cut a release. Do you mind rebasing and resolving the conflict?
If the interspersed option is set false and one arg is already set all following arguments are counted as arg and not parsed as flags. Because of that we should not offer flag completion. The same applies to arguments followed after `--`. Signed-off-by: Paul Holzinger <[email protected]>
@jpmcb done |
If the interspersed option is set false and one arg is already set all
following arguments are counted as arg and not parsed as flags. Because
of that we should not offer flag completion. The same applies to arguments
followed after
--
.Fixes #1307