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: completions command #1157
base: main
Are you sure you want to change the base?
Conversation
Pushed a change to add the current completion scripts to the templates. I've also added some fixtures for the scripts so that its easier to review changes when editing the templates. Simply run I've been thinking a little bit about the implementation and we have the following decision to make:
I'm really undecided between the two. It might be possible to get the best of both worlds by creating a 2nd template layer. i.e. Either way, this can be merged as-is and the rest of these decisions can be made in future PRs. Also, apologies in advance, but I'm going to be annoying and tag some of the users who have contributed to completions in the past. It would be great to get your perspectives on the changes too. @carlsmedstad @MarioSchwalbe @patricksjackson @mymmrac @notnmeyer @shilangyu @paulvarache @trim21 @philpennock |
I'm a windows user and current powershell completion script (backed by and I agree your opinion on cobra, cobra assume your cli have sub commands, which task doesn't. So migrating to cobra just for auto completion is not a good idea. |
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.
LGTM! I think this is a helpful change, since some completion scripts are using external tools like sed
, read
or etc. to parse tasks list, so it may not work on others PCs.
Also, this is great, becouse for example
sed
expressions used in fish complition script are pretty complex right now 🙁
I just submit a PR for powershell completion, and I find that parsing json in powershell may cause feelable delay. So now I'm not very sure about option 1. Are you suggesting to handle current user input (filter flags/ tasks) in go instead of shell? Or just generate a script with full tasks list, then filter them in shell? Generating flags is always a good idea. |
I didn't follow the development and discussion, so I'm a little confused by this thread. why
and use I think it's already covered by Did I miss something here? generate:
desc: Runs Go generate to create mocks
aliases: [gen, g]
Is this referring to support variable completion in the future? |
Don't forget
If merged this would currently break at least go-task/homebrew-tap but probably also other downstream packages. The quick solution is to what you do with With regards to the two other options I believe
Is the way to go - it matches what we currently do. We can come up with some additional tricks to make completions even better in the future. Just getting the templating in place for now is a big win in my eyes. I'll for sure take a crack at improving the bash completions when this is merged. |
Fixes #293
A draft PR based off this comment.
It's a pretty simple implementation. We have a new internal package called
completion
. This will embed any files in theinternal/completion/templates/*
directory and when the--completion <shell>
flag is given, it will read the template with the corresponding template name, insert any template values and return the string which will then be output to stdout.I've added a
task.tpl.example
file to illustrate how you would add a completions script. I'm not super proficient with completion scripts (I have a bit of experience with ZSH), so maybe if we're happy with this approach, we can merge with no templates and slowly migrate the old scripts to this new concept.I wasn't sure what the best format for the template filenames was. I think we want them to end with
.zsh
/.bash
etc so we get syntax highlighting, but I also added the.tpl
prefix so that anyone using a go templating extension/plug could add.tpl.bash
etc to the list of templated files without also targeting normal.bash
files. Let me know if you think there's a better way?