-
Notifications
You must be signed in to change notification settings - Fork 321
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
Fix zsh/bash completions for arguments in option groups with a custom completion #648
Fix zsh/bash completions for arguments in option groups with a custom completion #648
Conversation
This resolves an issue where custom completion for arguments in an OptionGroup would fail to match the argument. It was caused by: - the completion script only using the name of the argument (instead of the full path) - the CommandParser looking for the matching argument by comparing a name only IndexKey with the “full” IndexKeys
The zsh completions already uses this function. The function’s implementation is the same as what the BashCompletionsGenerator is doing. This removes the duplicated logic.
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.
Looks good, thanks for working on this! One nit, then we should be good to merge.
Have you had a chance to see if this behavior occurs with fish
completions as well?
Prevously was using .components(seperatedBy:) from Foundation.
👍 That's been pushed to the branch.
From what I can tell, it appears that completions for arguments aren't being generated currently for fish. This is returning
It'll take me some more time see if there's a reason for the condition in the guard and what the solution would be. |
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've added a commit that adds the arguments to the fish completions. I tried implementing the logic to follow the bash/zsh implementations to control which arguments are shown/hidden.
@@ -608,23 +608,30 @@ function _swift_math_using_command | |||
end | |||
|
|||
complete -c math -n \'_swift_math_using_command \"math add\"\' -l hex-output -s x -d \'Use hexadecimal notation for the result.\' | |||
complete -c math -n \'_swift_math_using_command \"math add\"\' -l version -d \'Show the version.\' |
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.
The completions now include the --version
flag like the bash/zsh commands.
complete -c math -n \'_swift_math_using_command \"math stats\" \"average stdev quantiles help\"\' -f -a \'average\' -d \'Print the average of the values.\' | ||
complete -c math -n \'_swift_math_using_command \"math stats\" \"average stdev quantiles help\"\' -f -a \'stdev\' -d \'Print the standard deviation of the values.\' | ||
complete -c math -n \'_swift_math_using_command \"math stats\" \"average stdev quantiles help\"\' -f -a \'quantiles\' -d \'Print the quantiles of the values (TBD).\' | ||
complete -c math -n \'_swift_math_using_command \"math stats\" \"average stdev quantiles help\"\' -f -a \'help\' -d \'Show subcommand help information.\' |
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.
The changes seems to have also fixed another bug where it could suggest help
following subcommands when it isn't a valid subcommand.
@swift-ci Please test |
Issue
Description
The issue
It was caused by a mismatch in the completion scripts and the custom completion parsing. This is following the example from the issue above.
The completion scripts contained the name of the argument
':arg-in-option-group:{_custom_completion $_completion_example_commandname ---completion -- argInOptionGroup $words}'
The parsing of the completion command failed to find the argument using
ArgumentSet.firstPositional(named:)
.This was caused by the function creating an
InputKey
with just aname
and comparing to keys in the set that have theirname
andpath
set.In other words
There's also another issue that would occur when a command and an option group each contain an argument with the same name. In that case the completion would always match the first one in the
ArgumentSet
even if the user is trying to get completions for the second argument.The fix
The completions scripts were updated to contain the full path of the argument instead of just the name. Then the completions parser can create an
InputKey
that will exactly match one in theArgumentSet
.To avoid having the logic spread out, an extension was added to
InputKey
to get thefullPathString
from a key and an init to create anInputKey
from afullPathString
.I also updated the bash completion script to use
ArgumentDefinition.customCompletionCall(_:)
that the zsh script was already using to deduplicate the logic.Checklist