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
feat: add command completion #650
feat: add command completion #650
Conversation
Hi @prodanlabs, This pr is great! Please fix the ci error, then we will finish the review and merge as soon as possible! |
@imxw Would you please help to take a review at this pr too? |
Of course, it's my pleasure. |
cc /@daniel-hutao |
cmd/devstream/completion.go
Outdated
return cmd | ||
} | ||
|
||
func completionBash(out io.Writer, cmd *cobra.Command) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We follow the Standard Go Project Layout, and the cmd
directory is better not to contain a lot of logic:
Don't put a lot of code in the application directory. If you think the code can be imported and used in other projects, then it should live in the /pkg directory. If the code is not reusable or if you don't want others to reuse it, put that code in the /internal directory. You'll be surprised what others will do, so be explicit about your intentions!
Based on this, I suggest we move the code from line 63 to the end of this file to another place, like internal/pkg/completion/xxx.go
(only an example/a suggestion.)
cmd/devstream/completion.go
Outdated
"github.com/spf13/cobra" | ||
) | ||
|
||
// completionCMD represents the completion command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a little bit redundant information. I think it's fine if we don't have this comment at all.
@prodanlabs thanks for this great feature. I reviewed and added two little comments. |
After some testing, there is another comment I would like to add: On macOS, given how bash-completion is installed, the default location of completion scripts isn't necessarily always For example, if auto-completion@2 is installed with brew, it seems the default location is So, it's not accurate to put a default location (especially for macOS, considering how many people are using brew) in the help doc. Maybe we can give some extra information in the help doc. Another issue is, there are two versions of bash-completion, v1 and v2. V1 is for Bash 3.2 (which is the default on macOS), and v2 is for Bash 4.1+. I didn't test if the dtm completion script works with Bash 3.2 or not. I think it's best to tell macOS users how to upgrade to Bash 4.1+ (maybe brew install) and to install auto-completion@2. K8s has a decent doc on auto-completion: https://kubernetes.io/docs/tasks/tools/included/optional-kubectl-configs-bash-mac/ We could also write a blog about it. Are you interested? @prodanlabs |
Changes have been made as suggested by the comments, but I haven't seen this style |
Personally, I think |
@prodanlabs Thanks for your contribution, I like your idea! After testing, I have left a few comments. In addition, I wonder if you'd be interested in implementing this completion feature, where [TAB][TAB] only shows yaml type files when using |
cmd/devstream/completion.go
Outdated
func completionCMD(out io.Writer) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "completion", | ||
Short: "Generate the autocompletion script for dtm for the specified shell.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a problem, but to unify other help information styles, I don’t know if you mind removing the period at the end of the help info.
cmd/devstream/list.go
Outdated
if len(args) == 1 { | ||
var choices []string | ||
for _, pluginName := range list.PluginsNameSlice() { | ||
if strings.HasPrefix(pluginName, args[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of dtm list plugins
is to show all available plugins, so why filter the specified plugins here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as dtm list plugins -r
, filter plugin by regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as
dtm list plugins -r
, filter plugin by regex.
There is one difference, the result of dtm list plugins -r
is the result of regular matching, but here only the operation [TAB][TAB] will filter out the relevant plugin name, the execution of the command does not affect the output, it is still all available plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, do [TAB][TAB] to match or complete. complete is always like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, do [TAB][TAB] to match or complete. complete is always like this.
Yeah, but plugins here are not args or subcommands. It's just like kubectl plugin list
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the problem. When the user wants to know how many plugins are related to argocd
, they can simply do dtm list plugins argocd[TAB][TAB]
to filter them out. I believe this is also the purpose of dtm list plugins -r
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prodanlabs the only ways to use the list plugins
subcommand are:
dtm list plugins
which shows all the pluginsdtm list plugins -r argo
which shows the plugins whose names match "argo"
So, even if the user types dtm list plugins argo [tab][tab]
and it shows dtm list plugins argocd
, it's not useful, because the user can't execute this command.
|
||
Load completions for every new session: | ||
(in Linux)# %s completion bash > /etc/bash_completion.d/dtm | ||
(in MacOS)# %s completion bash > /usr/local/etc/bash_completion.d/dtm`, binary, binary, binary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/usr/local
can be changed to $(brew --prefix)
, because only Mac computers with Intel processors is /usr/local
, but M1 is /opt/homebrew
I don't think it makes much sense, to be honest. |
Configuration file completion
|
It's okay, it's not mandatory to achieve. |
} | ||
} | ||
|
||
func FlagConfigFileCompletion(cmd *cobra.Command) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before, it's not mandatory to achieve and you also didn't think it is necessary to achieve, but since you have achieved it, I refer to the official documentation and found that this implementation is not applicable to Fish
and PowerShell
Fish doesn't support BashCompFilenameExt
Powershell doesn't support BashCompFilenameExt
A more generic implementation is as follows.
Of course, if you still don't think it's necessary to implement this feature, you can just remove the relevant code. Thanks again.
"github.com/devstream-io/devstream/pkg/util/log" | ||
) | ||
|
||
func ArgsPluginsCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the problem. When the user wants to know how many plugins are related to
argocd
, they can simply dodtm list plugins argocd[TAB][TAB]
to filter them out. I believe this is also the purpose ofdtm list plugins -r
.
I agree that we need to allow users to filter plugins. So dtm list plugins -r
should be accepted. However, in my opinion, the premise dtm list plugins argocd[TAB][TAB]
can be accepted is that dtm list plugins
accepts the plugin name as arg or subcommand. I don't know if you understand what I mean.
I think dtm list plugins -r xxx
is good. The result can be used as stdin
for other commands. For example
dtm list plugins -r "\bargocd\b" | dtm show config
dtm show config $(dtm list plugins -r "\bargocd\b")
The results of dtm list plugins argocd
are the same as dtm list plugins
. In addition, we have to clean up the command manually before executing the next command.
In summary, why do we need dtm list plugins argocd[TAB][TAB]
.
What do you think? @daniel-hutao @IronCore864
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the dtm list plugins argocd
command does not exist; dtm list plugins
is used to print all plugins, and this command has no further subcommands, it only has flags, like -r
. So dtm list plugins [tag]
should not output specific plugin names.
The current zsh completion generated by cobra does not work, but the problem has been solved in this pr, we can fix it by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This discussion took too long, everyone worked hard!
@imxw please help to add the documentation after this pr is merged in.
Signed-off-by: prodan <pengshihaoren@gmail.com>
Please continue to follow up on this issue, I suggest using cobra stable version, please wait for the |
Signed-off-by: prodan pengshihaoren@gmail.com
Pre-Checklist
Note: please complete ALL items in the following checklist.
Description
Auto-completion help, take
bash
as an example1、
Flags
autocomplete2、
Args
autocomplete3、Autocompletion when binary name is not
dtm
Related Issues
#642
New Behavior (screenshots if needed)