Open marcusirgens opened 4 years ago
Hey @marcusirgens, thanks for the PR :tada:
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.
Hey, thanks for the feedback. I agree with your comments, I’ll make some changes and update this PR.
Adds support for zsh descriptions as requested in #63