Skip to content

Refactor completion script generation to use ToolInfoV0 #764

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
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rgoldberg
Copy link
Contributor

@rgoldberg rgoldberg commented May 7, 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. It builds, and passes the format check,
but the generated completion scripts for each shell aren't what they should be, I think because 3
issues that I think require ToolInfoV0 to be updated. The test scripts contain what I believe to be
the desired output.

The 3 issues that I think exist in ToolInfoV0 are:

  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 this PR no longer substitutes _ in place of - 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 out that change into a separate PR, if necessary…

Please let me know if the problems I've mentioned are indeed in ToolInfoV0, or if I'm just using it wrong.

Also, please let me know who might be able to fix ToolInfoV0, and if they'll be able to do so anytime soon.

If it won't take too much time, it would be nice to get this in to 1.6.0.

Also, for the 1.6.0 release notes, I'd very much appreciate it if you could mention that it is known that preexisting bugs from 1.5.0 still exist in completion script generation, and that they will be fixed in subsequent releases. I don't want people to think that I think that the current state is close to bug free. Thanks.

Resolve #758 #780

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

@natecook1000 @rauhul Do my theorized issues seem to exist in ToolInfoV0? Or must I modify my PR in some way instead of ToolInfoV0 being changed?

If the former, will anyone else work on ToolInfoV0 updates soon? I'm working on some other completion fixes, so I'd prefer the code not to diverge too much, and I'd prefer to work using the nicer ToolInfoV0 interface, if possible.

If you don't have the bandwidth to handle ToolInfoV0 now, I completely understand.

@rauhul
Copy link
Contributor

rauhul commented May 13, 2025

@rgoldberg sorry about the radio silence, speaking for myself im super super busy leading up to early June and can't take a close look at SAP changes until deadlines pass. Sorry :(

@rgoldberg
Copy link
Contributor Author

@rauhul No problem. Completely understandable.

If no one else can look into ToolInfoV0, then I might delve in.

Good luck with your other work!

@rgoldberg rgoldberg marked this pull request as ready for review May 13, 2025 18:13
@rgoldberg
Copy link
Contributor Author

rgoldberg commented May 13, 2025

@rauhul @natecook1000 I confirmed & fixed the 3 issues that I identified in ToolInfoV0. I don't know if my changes will cause any issues elsewhere for ToolInfoV0, but all the tests pass (after I applied some updates to expected test output that all seem correct to me).

I have not looked into the SAP-wide issue because that existed before the attempt to switch to ToolInfoV0. I opened #771 for that.

Thanks again.

@rgoldberg rgoldberg changed the title [WIP] Refactor completion script generation to use ToolInfoV0 Refactor completion script generation to use ToolInfoV0 May 13, 2025
@rgoldberg rgoldberg force-pushed the 758-tool-info branch 3 times, most recently from 9de430b to d11f4b0 Compare May 13, 2025 23:55
rgoldberg added 5 commits May 15, 2025 01:12
…arate ArgumentInfoV0 instances, instead of different names for the same ArgumentInfoV0.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Copy link
Contributor Author

rgoldberg commented May 17, 2025

@jaredgrubb This PR already fixes #780, so #781 should be unnecessary. I think the solution here is more optimal, but, if people disagree, please let me know.

If people want to hold off on the ToolInfoV0 refactor, but still want #780 solved earlier, I think it makes more sense to extract the solution from here into a new PR than to use #781.

Also, I noticed that the shellVariableNamePrefix Swift computer var isn't used anymore, so I've pushed an extra commit to this PR to remove it.

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.

Refactor completion script generation to use ToolInfoV0
2 participants