Conversation
|
will squash the commits after review changes |
47eae3b to
efdf919
Compare
2b592cb to
413dd7f
Compare
663d071 to
35923ba
Compare
f3341eb to
c4795fe
Compare
|
Yes! This (specifically alias completion) is the thing that will make gh the perfect tool for me ✨ - was about to report an issue but found a fix was there already. Thank you for pushing this @rsteube |
c4795fe to
d09d4c9
Compare
|
@kvz got prebuilt binaries at https://github.com/rsteube/gh/releases if you want to try it out |
|
@rsteube Thank you for keeping this branch updated! This is definitely one big PR, and I ask for your patience while we review and make a decision on the changeset. 🙇 |
|
@mislav Yes, that is not unexpected. Took some shortcuts here and there as a couple of things weren't easy to get working. You probably also want to have a look at carapace itself to decide if it is something you want to add as dependency or not. Some sidenotes:
|
|
@rsteube How does Carapace compare to using Cobra's |
|
@mislav Have to look at it myself as i didn't keep much track of it for a while.
Carapace just has a few more features and some issues seen in the cobra completion fixed:
Things have gotten pretty stable with the features working rather consistently in each shell in carapace so i'd say it's still a bit ahead though. |
|
Ok this is interesting, since the api is so similar adding the carapace completions to cobra isn't too hard 😄 : func (c Carapace) FlagCompletion(actions ActionMap) {
for name, action := range actions {
if flag := c.cmd.LocalFlags().Lookup(name); flag == nil {
fmt.Fprintf(os.Stderr, "unknown flag: %v\n", name)
} else {
actionMap[uid.Flag(c.cmd, flag)] = action
c.cmd.RegisterFlagCompletionFunc(name, func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
rawValues := action.Invoke(Context{Args: args, CallbackValue: toComplete}).rawValues
result := make([]string, len(rawValues))
for index, r := range rawValues {
if r.Description != "" {
result[index] = fmt.Sprintf("%v\t%v", r.Value, r.Description)
} else {
result[index] = r.Value
}
}
return result, cobra.ShellCompDirectiveDefault
})
}
}
}Will have a look at what it can do by now, but i can already see some issues. |
d09d4c9 to
094595f
Compare
d7be723 to
545cc53
Compare
2c87a41 to
c21bbae
Compare
002b621 to
07aedf0
Compare
07aedf0 to
ff19fb7
Compare
|
@rsteube Thanks for continuing to work on this. Due to the size of this change (3000 LOC!) and the tight coupling of many pieces of our codebase to a relatively young library (Carapace), I do not think that we will ever merge this as-is, but I keep perusing your code and I learn something new along the way every time. I especially like seeing how you've added completions for flag values based on data backed by the API. ✨ I've recently made a tiny PR to add completions for the I'm thinking that an incremental approach like that, backed by a framework that we already use (Cobra), would work better for us in the long run. Based on your experience so far, do you see any limitations with that approach? |
|
@mislav Yes i expected that we would have to split this up due to the size, it was just simply easier to keep track of the changes and test it. I think for the most part it will work quite alright with cobra's completion but from what i could see when i tested it there are still a couple of issues with it. Limitations you might encounter (as i did during the development of carapace):
Nothing too serious though, but expect fixes regarding this to take a while in cobra. Also be sure to limit the requests to the graphql api to return only what you need as this has quite an impact on the completion delay. I will probably transform this to a standalone completer then where the completion is provided by a separate binary like here. Actually this is already working as is, but does not make too much sense to bundle the whole application for which it provides the completion. |
|
@mislav moved it here as standalone completer: https://github.com/rsteube/carapace-bin/tree/master/completers/gh_completer/cmd |
|
Mega-thanks for all this!
But bash doesn't have native support for descriptions, right? I appreciate you being to add them, but I feel that if people want richer shell completions, they should move to a shell that offers richer completion support (e.g. zsh, fish).
This is a great point. We definitely don't want API-backed shell completions to hit the API repeatedly. 👍 |
As discussed in #360 this adds consistent dynamic completion for bash, elvish, fish, oil, powershell, xonsh and zsh.
Left the current completion command untouched as a couple minor issues are expected when used in different environments.
Completion script is generated by calling
gh _carapace [shell](shell is optional and will be detected by parent process name).Added Dockerfile/docker-compose with the various shells to test the completion (needs
GITHUB_TOKENin.env).Got some documentation about carapace at https://rsteube.github.io/carapace/ (though still very basic and needs to be updated for the recent changes).
Working
...
Missing:
exec.Command...
fix #360
related #695
related #1128
related #1775
related #2471