@Galileo927 @pintaste @kiwigitops @natecook1000 @rauhul @Roshankumar350

I've looked at both recent PRs for this: #898 & #900.

After locally converging the PRs' code, they're the same except for:

their testing (which I haven't yet read)
Include hidden arguments in completion scripts #898 trying to fix preexisting logical issues around completing non-default arguments

Note that I hadn't previously thought about the preexisting logical issues around completing non-default arguments, as I regarded visibility as outside of the scope of my updates, so I accepted the existing exclusion of all non-default as given.

Before progressing with reviews / improvements, we should all agree on the issues at hand. I see:

1

It seems to me that if any completion: parameter is explicitly provided for any argument—regardless of argument type (@Argument, @Flag, @Option, etc.) or visibility—the completion should be used.

Does anyone disagree?

2

It seems that implicit completions should:

be used for default visibility (as existing behavior)
be used for hidden visibility (as per this issue)
not be used for private visibility (as existing behavior)

Does anyone disagree?

3

I haven't completely digested the code, but I assume implicit completions are used if ArgumentDefinition.completion == .default & if SAP can determine the possible values for the argument. If no completion: parameter is passed to a wrapper, it seems that ArgumentDefinition.completion == .default.

To distinguish between explicit & implicit use of .default, I propose adding public static var implicit: CompletionKind, which would be used iff no completion: parameter value (or nil) is passed to a wrapper. Users would use the existing completion: .default if they want to explicitly use default completions for all visibilities, or .implicit for just hidden or default visibility.

Or, public static func default(minimumVisibility: Visibility) could be added instead of .implicit, with an associated visibility value added to the associated enum case. Existing implicit use of .default would switch to .default(minimumVisibility: .hidden). This is probably better than .implicit.

Does anyone disagree?

4

Regardless of the above, it seems that all positionals (@Argument)—regardless of visibility—must be taken into account when determining indexes for positional custom completion requests. Currently, this isn't properly handled in all shells, if any. This might be fixed by #898 (I need to digest all the code).

Does anyone disagree?

5

Should all of this be handled in one PR?

6

Are there any other related issues?

Uh oh!

Include hidden args in completion scripts #733

Description

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions