Skip to content

Improve zsh completion script generation #727

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 33 commits into from
Feb 15, 2025
Merged

Improve zsh completion script generation #727

merged 33 commits into from
Feb 15, 2025

Conversation

rgoldberg
Copy link
Contributor

Improve zsh completion script generation.

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

This PR is for a first batch of zsh completion script fixes.

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

If you want documentation of the fixes directly in the GitHub issue #726 and/or in this PR without having to look at the commit messages, I can include info from the commit messages here and/or in the issue description.

If you want the this PR split up, parts removed or changed, etc., please let me know, and I'll make the requisite changes.

After this PR is merged, I will sequentially create an issue & submit a PR each for initial bash & fish fixes, then one or more PRs for cross-shell changes, then I will resume working on more fixes.

Resolve #726

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

@natecook1000
Copy link
Member

@swift-ci Please test

natecook1000
natecook1000 reviewed Feb 13, 2025
View reviewed changes
Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this awesome work, @rgoldberg! From testing it looks flawless, just have a few minor notes/questions below.

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Feb 13, 2025 •
edited
Loading

@natecook1000 Thanks for the feedback. Do you want me to update things via new commits, or by fixing existing commits & force pushing? I assume the former, since it seems like SAP squashes PRs, but I wanted to check with you first.

I will push new commits with fixes so I can resolve conversations here with code having been submitted. If you want me to redo existing commits then force push to keep the PR commit history cleaner, let me know; if so, I'll move the changes from the new commits to the original commits, then force push.

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Feb 14, 2025 •
edited
Loading

@natecook1000 I've submitted fixes in new commits for the conversations that I've resolved. Waiting for your instructions on the open conversations. Thanks for all the feedback & help.

@rgoldberg
Copy link
Contributor Author

Rebased on the new main as there is a simple conflict that needed to be resolved.

@natecook1000
Copy link
Member

Feel free to handle commits/updating however's most comfortable for you – we'll squash on merge, so the in-process commits aren't terribly important.

@rgoldberg
Do not indent zsh cases.
5f5446b 
Simplify zsh indent generation.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Do not prefix zsh cases with an open parenthesis.
e22f251 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Prevent zsh parameter word splitting.
beaa419 
Brace & quote parameter uses.

Use [@] for quoted array output.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Improve comment in ZshCompletionsGenerator.swift.
480f772 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Fix incorrect zsh shellCommand single quotes:
0eae18f 
4 consecutive single quotes were obviously intended to be 2 escaped single quotes, but that isn't zsh syntax.

Use 2 double quotes instead.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Remove extraneous zsh newline.
b4b492a 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Improve zsh variable declarations: scoping, typing & readonly.
dbbb99e 
Remove trailing spaces from InstallingCompletionScripts.md.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Include zsh words before current subcommand in custom completion arg.
69f2d73 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Make subcommandHandler in ZshCompletionsGenerator.swift immutable.
17012b5 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Escape zsh single quotes via '\'' instead of via '"'"'.
a6fd712 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Escape single quotes in zsh shellCommand String.
7df50be 
If someone already escapes single quotes from the String, this will cause an issue, but no one should be required to go to the trouble to manually escape single quotes in their script, especially since the requirement isn't documented or normal.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Fix zsh custom completions for empty [String] & String elements.
ea1047f 
If a Swift custom completion function returns an empty [String], if the user tries to complete it, refuse to complete instead of inserting a blank space into the command line.

If a Swift custom completion function returns a [String] including a Swift empty String or including a String with a description but with a blank completion (e.g., ":description"), if that completion is selected, complete to a zsh empty string '' instead of inserting a blank space into the command line.

Disambiguating between an empty [String] & a [String] with one empty String element requires that an extra value be appended to the output of the Swift custom function, which is then removed by the completion script.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Simplify zsh subcommand completion function dispatch.
ded075d 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Restrict access to symbols in ZshCompletionsGenerator.swift.
28a6b12 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Add default help to zsh completions iff no existing help subcommand.
33fbfbc 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Use interpolated Strings in ZshCompletionsGenerator.swift.
72eb64c 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Create & use zsh __completion function.
83fa27c 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Improve zsh escaping.
0f8b5ac 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Set zsh settings to a known state.
d278fd1 
Disable history ! in zsh completion scripts.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Inline single-use functions & variables in ZshCompletionsGenerator.sw…
1d6e52c 
…ift.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Overhaul ZshCompletionsGenerator.swift as [ParsableCommand.Type] exte…
fc8b1d5 
…nsion.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Move functions in ZshCompletionsGenerator.swift.
afecd9c 
Move from ArgumentDefinition extension to [ParsableCommand.Type] extension.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Move zsh helper functions before command functions to mirror other sh…
a2743b1 
…ells.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Prefix zsh helper functions with command name to prevent naming clashes.
ea62664 
Function names are globally scoped.

Without namespacing, if 2 programs use different versions of Swift Argument Parser, one could overwrite the other's different version of the same helper function.

Renamed functions from *_completion to *_complete, as they complete, not return a completion.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Separate zsh _arguments flags from specs using :.
15bbfa0 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Rename zsh args variable as arg_specs.
f87085c 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Simplify zshCompletionString(…).
697e5e8 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Allow generating zsh setup scripts for arguments.
c6d1375 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Use zsh array for list completions instead of nested strings.
e8ca77d 
Allows list completions to contain spaces.

Resolve apple#726

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Make CompletionShell.format(…) internal instead of public.
8583ab6 
Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Reword uses of "iff" in completions code.
ecb43b9 
Redid a comment as a DocC.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Replace zsh END_MARKER pseudo-completion with a space to ease migration.
19e4df1 
Document why & how this pseudo-completion is used.

Do not trim whitespace in testing, as that breaks with the space pseudo-completion.

Testing should be as exact as possible; trimming whitespace makes it less exact.

Signed-off-by: Ross Goldberg <[email protected]>
@rgoldberg
Throw error if attempting to generate a zsh completion script for no …
6d32ef3 
…commands.

Force unwrap first in ZshCompletionsGenerator.swift.

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

@natecook1000 I've finished all the fixes. Please note that I cannot move the members from the extension Sequence to the extension […] because in some not-yet submitted commits for bash completion script generation, I need one of the members in extension Sequence, so I should leave both in case we need the other there in the future, too.

I've also rebased on main.

natecook1000
natecook1000 reviewed Feb 15, 2025
View reviewed changes
Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look great! Just a little bit of cleanup, then we're good to merge 🎉

@rgoldberg
Copy link
Contributor Author

@natecook1000 Just replied to your 2 comments.

@natecook1000 natecook1000 enabled auto-merge (squash) February 15, 2025 18:52
@natecook1000
Copy link
Member

@swift-ci Please test
@swift-ci Please test

@natecook1000 natecook1000 merged commit f2eda39 into apple:main Feb 15, 2025
22 checks passed
@rgoldberg rgoldberg deleted the 726-zsh-completion branch February 15, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@natecook1000 natecook1000 natecook1000 left review comments

@rauhul rauhul rauhul left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve zsh completion script generation

3 participants

@rgoldberg @natecook1000 @rauhul