Skip to content

Refactor completion script generation to use ToolInfoV0 #758

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

Open
rgoldberg opened this issue Apr 27, 2025 · 3 comments · May be fixed by #764
Open

Refactor completion script generation to use ToolInfoV0 #758

rgoldberg opened this issue Apr 27, 2025 · 3 comments · May be fixed by #764

Comments

@rgoldberg
Copy link
Contributor

rgoldberg commented Apr 27, 2025

@rauhul @natecook1000

Refactor completion script generation to use ToolInfoV0 for all 3 supported shells: bash, fish &
zsh.

@rauhul previously submitted PR #695 that refactored bash. He graciously sidelined his PR to
allow me to merge my numerous pending completion improvements, which have now all been
merged.

My WIP draft PR refactors all 3 shells to use ToolInfoV0, except it has 3 issues that I think require ToolInfoV0
to be updated:

  1. ToolInfoV0 must include parsingStrategy in ArgumentInfoV0 (needed for zsh).
  2. The HelpCommand that is injected into ToolInfoV0 has a --version flag, but it is not seen
    by the completion code. Might be useful to try to resolve all issues from Problems with auto-created --help flag, -h flag, & help subcommand in generated completion scripts & help output #671, too.
  3. Nonexclusive flags implemented via an array of enum cases are represented as different names
    for the same ArgumentInfoV0, but each case should have its own ArgumentInfoV0. If such
    enum cases can have multiple names each, then all names for a single case should be in the
    same ArgumentInfoV0. Example enum & property (from CompletionScriptTests.swift):
    enum Kind: String, ExpressibleByArgument, EnumerableFlag {
        case one, two
        case three = "custom-three"
    }
    
    @Flag var allowedKinds: [Kind] = []

Bonus issue: the following seems similarly inconsistent in all of SAP that I've seen, not just
in ToolInfoV0. case three = "custom-three" from above is considered as --three for a flag
from allowedKinds, but as custom-three as an option value for --kind from
@Option() var kind: Kind. This is the case in the original completion scripts, in ToolInfoV0
& even in SAP's command-line argument verification. It seems to me that it should be consistent
throughout; of the 2 options, custom-three seems more sensible.

ToolInfoV0 and/or SAP might have other issues, but I haven't isolated them.

Note that my PR for this no longer replaces - with _ in bash shell function names because:

  • it's unnecessary (- is supported in function names; - is not supported only in variable names)
  • bash is now consistent with the other 2 shells, and can use the same Swift code to generate function names
  • it avoids (albeit exceedingly unlikely) potential name clashes between, e.g., subcommands named a-b & a_b

I can split it out into a separate PR, if necessary…

@rgoldberg
Copy link
Contributor Author

@natecook1000 I saw that you had CI test the fish PR.

Do you want me to submit the WIP PR for this so you can look at it in the meantime, or do you just want me to sequentially submit PRs for #739 (just docs), #756 (small) & #757 (2 extra index arguments) after the fish PR has been merged?

@natecook1000
Copy link
Member

Merged the fish PR – whichever way works for you to get these up and running would be great! I think a natural place for a new release (will be 1.6.0) is after #757 lands, so I'll watch for your PRs and get them ushered through as you're able to submit.

@rgoldberg
Copy link
Contributor Author

@natecook1000 So you want to release 1.6.0 without a PR for this issue?

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 a pull request may close this issue.

2 participants