Skip to content
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

📺 Add zsh completions #514

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

📺 Add zsh completions #514

wants to merge 2 commits into from

Conversation

Logicer16
Copy link

#222 has been open for over 5 years now (🥳 congrats). About time someone took care of it, especially as it's the default shell.

Closes #222

Please let me know of any questions/suggestions you might have.

contrib/completion/mas-completion.zsh Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Show resolved Hide resolved
contrib/completion/mas-completion.zsh Show resolved Hide resolved
@Logicer16
Copy link
Author

Let me know if any of these reviews are an actual concern as most aren't applicable.

contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Show resolved Hide resolved
@Logicer16 Logicer16 force-pushed the main branch 2 times, most recently from c6db308 to 63d0fe8 Compare May 17, 2024 10:13
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
contrib/completion/mas-completion.zsh Outdated Show resolved Hide resolved
@Logicer16 Logicer16 marked this pull request as ready for review May 17, 2024 10:59
@Logicer16
Copy link
Author

Been busy lately, but I've finally gotten time to come back to this pr.

I've changed it so the output from mas search, mas list, and mas outdated are all parsed into an array of ids and names such that an associative array can be constructed from the data. The other functions now pick the data they need from this array.

__mas_list_outdated also no longer suggests the name or id of any apps which have already been specified.

Thanks for taking the time to review!

@rgoldberg
Copy link
Contributor

rgoldberg commented Sep 25, 2024

@Logicer16 Thanks. I'll check out the new version in a few days.

I am currently working on switching from Commandant to Swift Argument Parser for multiple reasons, one of which is that it can autogenerate completion scripts for zsh, bash, and fish.

Once I'm done with it locally (I won't open any new PRs until an existing PR that fixes a breaking lint issue in main is merged), I'll compare the generated script to yours. If yours is better, I'll see if we can automate modifying the generated one to add your features, as it would be useful to be able to autogenerate competitions for all 3 shells whenever we release a new version instead of having to hand code things & to manually ensure that completion remains in sync with the executable.

@rgoldberg
Copy link
Contributor

rgoldberg commented Sep 26, 2024

@Logicer16 I have a working local branch in which I've switched from Commandant to Swift Argument Parser (SAP).

I've begun to look into SAP generating completion scripts for zsh, bash & fish.

It seems like the nicest way to provide custom completions for all 3 shells is to define them in Swift code. When wired via Swift attributes, the generated completion scripts call mas with special arguments to run the specified Swift function, so all of the logic only needs to be implemented once in Swift, instead of once each in zsh, bash & fish.

Moreover, we'll probably switch to outputting JSON from an internal Swift executable to an mas wrapper script. JSON will be formatted into tabular output in the wrapper, so even if we were to implement completion in shell instead of in Swift, we'd parse everything from JSON output instead of tabular output.

I'll leave this PR open, but we'll probably use the SAP completions.

Sorry that you did work that probably won't be merged, but you'll receive a much more maintainable mas this way.

No matter what, we need to wait for @phatblat to release new versions; I should have SAP completion script generation ready before we can release, so you shouldn't see any delay in getting zsh completion released between the SAP-generated script versus if we had merged your hand-coded completion.

@Logicer16
Copy link
Author

All good.

I agree, it would be much simpler to have everything defined in swift. Looking through their documentation, it should be possible to achieve feature parity with what's in this pr.

Regardless, with the amount I've learned about zsh in the process, I don't see this as time wasted.

Thanks for your time.

@rgoldberg
Copy link
Contributor

Thanks for your time & effort, too.

The SAP-generated zsh completion script is slightly wrong for the auto-created help subcommand, --help flag, and -h flag. I'm looking into reconfiguring the synthesized help functionality & completion, and into submitting PRs to SAP to fix the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zsh tab completion
3 participants