Skip to content
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

Merged

Conversation

CraigSiemens
Copy link
Contributor

@CraigSiemens CraigSiemens commented Jun 5, 2024

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 a name and comparing to keys in the set that have their name and path set.

    In other words

    InputKey(name: "argInOptionGroup", parent: nil)
    != InputKey(name: "argInOptionGroup", parent: .init(named: "optionGroup", parent: nil))

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 the ArgumentSet.

To avoid having the logic spread out, an extension was added to InputKey to get the fullPathString from a key and an init to create an InputKey from a fullPathString.

I also updated the bash completion script to use ArgumentDefinition.customCompletionCall(_:) that the zsh script was already using to deduplicate the logic.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary (wasn't necessary)

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.
@CraigSiemens CraigSiemens changed the title Fix zsh/bash completions for arguments in option groups for with a custom completion Fix zsh/bash completions for arguments in option groups with a custom completion Jun 5, 2024
Copy link
Member

@natecook1000 natecook1000 left a 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?

Sources/ArgumentParser/Parsing/InputKey.swift Outdated Show resolved Hide resolved
Prevously was using .components(seperatedBy:) from Foundation.
@CraigSiemens
Copy link
Contributor Author

Let's use the stdlib method here instead of Foundation's:

👍 That's been pushed to the branch.

Have you had a chance to see if this behavior occurs with fish completions as well?

From what I can tell, it appears that completions for arguments aren't being generated currently for fish.

This is returning nil if the ArgumentDefinition.names is empty.
https://github.com/apple/swift-argument-parser/blob/5be327c/Sources/ArgumentParser/Completions/FishCompletionsGenerator.swift#L62-L64

names will always be empty for positional arguments.
https://github.com/apple/swift-argument-parser/blob/5be327c/Sources/ArgumentParser/Parsing/ArgumentDefinition.swift#L109-L114

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.

Copy link
Contributor Author

@CraigSiemens CraigSiemens left a 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.\'
Copy link
Contributor Author

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.\'
Copy link
Contributor Author

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.

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit d66d015 into apple:main Jul 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zsh completion missing for arguments in an option group with custom completion
2 participants