Refactor Bash completions to use ToolInfo #695
Conversation
rauhul
force-pushed
the
rebase-bash-completion-on-tool-info
branch
3 times, most recently
from
04ba818 to
dae8159
Compare
Rebases the implementation of the BashCompletionGenerator to use ToolInfo from ArgumentParserToolInfo instead of digging through the command structure. This helps us decouple the implementation of Argument parsing from the generation of supplemental content such as docs, man-pages, completion scripts, help menus and more.
rauhul
force-pushed
the
rebase-bash-completion-on-tool-info
branch
from
dae8159 to
d66a07b
Compare
rauhul
changed the title
|
@swift-ci test |
|
@rgoldberg mind reviewing this PR? it should have minimal functional changes. |
There was a problem hiding this comment.
See this branch on my fork for recommended changes for BashCompletionsGenerator.swift that are local to the file (my other recommendations for the file require updating other Swift files, too, but those aren't in my branch). My recommendations that are local to other files are just in my review comments.
| @@ -191,9 +191,17 @@ extension ArgumentSet { | |||
| } | |||
|
|
|||
| func firstPositional( | |||
| withKey key: InputKey | |||
| withKey key: InputKey, | |||
There was a problem hiding this comment.
remove , because it breaks the build in Swift 5.7
| var result = [String]() | ||
| for argument in self.arguments ?? [] { | ||
| // Skip hidden arguments. | ||
| guard argument.shouldDisplay else { continue } | ||
| // Flags don't take a value, so we don't provide follow-on completions. | ||
| guard argument.kind != .flag else { continue } | ||
| // Skip if no keys. | ||
| let keys = argument.bashCompletionKeys() | ||
| guard !keys.isEmpty else { continue } | ||
| // Skip if no completions. | ||
| guard let completionValues = argument.bashOptionCompletionValues(command: self) else { continue } | ||
| result.append(""" | ||
| \(keys.joined(separator: "|"))) | ||
| \(completionValues.indentingEachLine(by: 4)) | ||
| return | ||
| ;; | ||
| """ | ||
| } | ||
| .joined(separator: "\n") | ||
| """) | ||
| } | ||
| return result | ||
| } |
| } | ||
|
|
||
| extension ArgumentInfoV0.NameInfoV0 { | ||
| func commonCompletionSynopsisString() -> String { |
There was a problem hiding this comment.
Given that the DocC for NameInfoV0 specifies the double & single dash prefixes, maybe move this extension into ToolInfo.swift, rename commonCompletionSynopsisString as synopsis, and change from func to var.
| @@ -139,7 +139,7 @@ _math_stats_quantiles() { | |||
| COMPREPLY=( $(compgen -W "$opts" -- "$cur") ) | |||
| } | |||
| _math_help() { | |||
| opts="--version" | |||
| opts="" | |||
There was a problem hiding this comment.
This should be opts="--version", as --version is a valid flag for math help.
| ) -> String? { | ||
| precondition(self.kind == .option) | ||
|
|
||
| switch self.completionKind { |
|
@rgoldberg formatting issue will be resolved in a future PR which enables swift-format, im going to mark them as resolved |
|
@rauhul @natecook1000 Is there any way to postpone modifying the completion script code & tests any more until after PRs for #679 have been merged? That will allow me to save a lot of time by not needing to drag massive changes through (currently) 85 commits; it took a while to drag through the formatting changes from #711 & the testing changes from #698, #699 & #700. It will take time to drag through changes from this PR (& from any potential similar PRs for zsh & fish). It would be easiest if all other completion work (including this PR) could be delayed until after #679 is done. I'm happy to submit an equivalent of this PR (& ones for zsh & fish) after #679 is done, to save you the trouble of having to do it. |
|
@rgoldberg Agreed on the order here – go ahead and get your changes lined up, and when that work has wrapped up we can do this migration. Thanks! |
|
@natecook1000 @rauhul My existing commits don't fix up all of the issues that I've found in completions. I assume that after my existing commits have been merged, it makes sense to migrate to ToolInfo, then resume completion fixes after that has been merged. Are you guys planning any other large-scale changes to completions or anything with which they interact? |
|
The goal is eventually to have everything going through a single path, rather than the separate generators for tool info, help, and completion scripts that we started with. That should both make things simpler to maintain and guarantee that we're including everything in |
Rebases the implementation of the BashCompletionGenerator to use
ToolInfo from ArgumentParserToolInfo instead of digging through the
command structure. This helps us decouple the implementation of Argument
parsing from the generation of supplemental content such as docs,
man-pages, completion scripts, help menus and more.