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 support for autocomplete descriptions #90

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marcusirgens
Copy link

Adds support for zsh descriptions as requested in #63

@stecman
Copy link
Owner

stecman commented Jan 24, 2020

Hey @marcusirgens, thanks for the PR 🎉

I like the direction this is going in - with a bit of work we can definitely fit this in.

My main concern with this initial API is that "description" is quite baked in, considering that only ZSH supports it. If someone wanted to add further metadata to command completions, they'd currently need to edit CompletionHandler directly which isn't ideal.

I'm also not a fan of CompletionResult representing a collection of results. I think it would make more sense as one instance per string so there's an obvious path to adding additional metadata. The current constructor that takes an associative array of name => description is fairly inflexible - for example, what if I wanted to add a color metadata value and set that on completions without using descriptions?

What I'd propose is:

  • Instead of setting the description in CompletionHandler, mark results with a type (eg. command, argument, option, option-value). Then deal with looking up command and option descriptions as needed in the ZSH output stage. User implemented completions should still be able to explicitly set descriptions, but it shouldn't be the responsibility of CompletionHandler to populate where this metadata is missing.

  • Rejig CompletionResult so it represents one item. The constructor can just take a single string, and (fluid?) methods can be used to set metadata.

  • Break CompletionCommand::writeForShell into a separate output module for each shell. Initially these wouldn't need to contain the hooks, but that's the eventual goal.

I'm happy to work through these with you, or if you want to smash it out by yourself that's cool too.

@marcusirgens
Copy link
Author

Hey, thanks for the feedback. I agree with your comments, I’ll make some changes and update this PR.

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.

None yet

2 participants