Refactor completion script generation to use ToolInfoV0

[rgoldberg]

@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]

@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]

@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]

@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]

@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]

@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.

[rgoldberg]

@natecook1000 If this PR isn't merged, then 1.5.1 will suffer from the bug from #780.

I've rebased this on main that includes your 1.5.1 changelog. I've also rebased #782 on it, too. #782 probably should be merged first, as it is simpler & you already OKed it for testing (I just had to fix a small issue with Linux testing, but it should pass that now).

Thanks again.

[rauhul]

@rgoldberg 1.5.1 isn't intended to have real functional changes, its just to unblock some windows use cases. We will want to release 1.6 with all of this big improvements.

[rauhul]

I'd like to avoid this. Instead the code generator should not emit trailing whitespace and should generate a trailing newline. We can make this its own GitHub issue to resolve later to avoid making this even more complicated.

[rgoldberg]

@rauhul I agree that generators should emit properly spaced output, but this might be useful to have in the meantime.

I'll defer to your judgement, but the reason why I added these is because, in the meantime, every time I must edit certain snapshots to match new generated output, I must edit the root .editorconfig to prevent my edits to the snapshots from automatically removing whitespace or adding a new line to the end of a file.

Sometimes I forget to do that. Sometimes I do that, but the changed .editorconfig gets committed. This happens most frequently when rebasing code to improve edit history to make it more understandable for code reviews, because the IDE I use offers to commit right after merging, but the merging doesn't allow you to edit the . editorconfig first.

Again, I'll do what you want, but it might be nicer for development to keep these new .editorconfigs, and to just have an issue for fixing up all the whitespace issues in generated output.

If you confirm that you still don't want this change here, I'll remove it. Can I just drop the commit, or do you want a separate commit that reverses the commit?

Thanks for reviewing.

[rauhul]

We can keep it for now, I agree its annoying to deal with the editor messing up the file during development.

[rauhul]

Can we factor the Toolset.jso changes into a different PR, that would be a lot easier to review in isolation.

[rgoldberg]

@rauhul The ToolInfo changes that result in JSON changes are necessary for the migrated completion scripts using ToolInfo to generate the same scripts as before (except for the fix to #780, which outputs improved scripting).

If you want, I can split it out into a new PR, but you can also just look at each commit of this PR individually, as they're sliced to be separate units of work described by their commit messages. The first 3 commits update ToolInfo; the next 3 commits update bash, fish & lastly zsh to use ToolInfo; the next commit removes a vestigial function; and the last commit adds the .editorconfigs.

If you want multiple PRs, can I put the first 3 commits in one PR? Or must I submit a separate PR per commit?

Can I keep bash, fish, zsh, function removal & .editorconfig addition in this PR?