Improve bash completion script generation #735

rgoldberg · 2025-02-15T17:28:27Z

Improve bash completion script generation.

There are many issues with script generation. Some of them are documented in #679 & its comments.

This issue is for a first batch of bash completion script fixes.

Each commit should note the fix(es) that it contains.

Note that I use "iff" in some Swift & bash comments, but not in code or in DocC. If you want "iff" removed from comments, please let me know.

Will rebase if main is updated (especially if #726 is merged).

Resolve #734

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

rgoldberg · 2025-02-15T19:37:26Z

@natecook1000 Just rebased on the new main with the zsh PR merged.

FYI: there are tons of TODOs in the bash completion code, as I haven't had a chance to get to all of the issues that I've seen. There are other issues that I've identified (in bash, fish, or zsh) for which I haven't included TODOs, too. I basically only noted issues in new code that I wrote, rather than in existing code; I completely rewrote the main completion logic for bash, so there is a lot of new code…

natecook1000

@rgoldberg This is great, again! 👏🏻 👏🏻 👏🏻 I really appreciate you starting from first principles for a rewrite of this size – the generated script is working without issues for me locally, so I just have these few implementation notes.

natecook1000 · 2025-02-15T19:11:38Z

Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift

+        \(positionalArguments
+          .compactMap { arg in
+            position += 1


I generally try to have a transform like this be a pure function, without the side effect of mutating a captured variable (i.e. position here). If you zip positionalArguments with an open range, you can iterate over both the position and the arguments at the same time:

Suggested change

\(positionalArguments

.compactMap { arg in

position += 1

\(zip(1..., positionalArguments)

.compactMap { position, arg in

I didn't know about Swift zip(…) before. Thanks for the tip. Being a Swift newbie has its drawbacks…

natecook1000 · 2025-02-15T19:14:04Z

Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift

-              \(functionName)_\(subcommand._commandName) \(subcommandArgument)
-              return
-              ;;
+    if !positionalArguments.allSatisfy({ bashValueCompletion($0).isEmpty }) {


What would you think about always doing the compactMap on lines 254+, and then, when that's not empty, adding the result plus the wrapper text (instead of this check)?

natecook1000 · 2025-02-15T19:17:30Z

Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift

-          ;;
-          """
+  /// Returns flags for the last command of the given array.
+  private var flags: [String] {


Thoughts on naming this something like flagCompletions instead? Reading the source above it looked like it would be the same type as positionalArguments (an array of ArgumentDefinition). Same thoughts for options, below.

natecook1000 · 2025-02-15T19:20:29Z

Sources/ArgumentParser/Completions/CompletionsGenerator.swift

+  /// Include default 'help' subcommand in nonempty subcommand list if & only if
+  /// no help subcommand already exists.
+  mutating func addHelpSubcommandIfMissing() {
+    if !isEmpty && allSatisfy({ $0._commandName != "help" }) {


Let's rewrite to flip the polarity:

Suggested change

if !isEmpty && allSatisfy({ $0._commandName != "help" }) {

if !isEmpty && !contains(where: { $0._commandName == "help" }) {

natecook1000 · 2025-02-15T19:28:57Z

Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift

+                      # TODO: handle joined-value options (-o=file.ext), stacked flags (-aBc), legacy long (-long), combos
+                      # TODO: if multi-valued options can exist, support them


Let's move these implementation notes out of the generated script – if there's a place for them in the Swift source, that's okay; otherwise let's just capture in issues.

Makes sense. I put the notes in situ since I used them as a guide for what to work on. Once the code is in GitHub, however, I can use code links in issues, which I couldn't do before.

natecook1000 · 2025-02-15T19:30:27Z

Sources/ArgumentParser/Completions/BashCompletionsGenerator.swift

+              fi
+
+              # ${word} is neither a flag, nor an option, nor an option value
+              # TODO: can SAP be configured to require options before positionals?


This isn't a feature currently – options/flags can come anywhere in the list of arguments.

rgoldberg · 2025-02-15T19:50:28Z

@natecook1000 Thanks for the feedback. Will try to get to it all tomorrow, then also submit a fish PR.

rgoldberg · 2025-02-16T03:50:06Z

@natecook1000 Subimtted fixes for all the comments.