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

feat: completions command #1157

Merged
merged 1 commit into from
Sep 2, 2024
Merged

feat: completions command #1157

merged 1 commit into from
Sep 2, 2024

Conversation

pd93
Copy link
Member

@pd93 pd93 commented May 10, 2023

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 the internal/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?

@pd93 pd93 force-pushed the completions branch 2 times, most recently from 4bb60e4 to 0674612 Compare May 15, 2023 12:32
@pd93
Copy link
Member Author

pd93 commented May 15, 2023

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 task gen:fixtures to update them.

I've been thinking a little bit about the implementation and we have the following decision to make:

  • Template includes Tasks in compilation and is static. This means task --completion <shell> must be called every time the user pushes TAB etc. However, it is easier to loop over tasks and their properties (such as aliases).
  • Template only includes tool-level info (i.e. flags) and not task-level info (i.e. Task names). This means it can be called once on shell startup and we will fallback to task --list-all --silent for fetching tasks inside the completion script as we do currently. We'd need to do some string parsing to get format the tasks and it will be more difficult to fetch task data (like aliases)

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. task --completion-tasks <shell> which would essentially replace task --list-all in the 2nd option.

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

@trim21
Copy link
Contributor

trim21 commented May 15, 2023

I'm a windows user and current powershell completion script (backed by task --list-all) already works perfect fine for me. So I don't have a opinion on this.

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.

Copy link
Contributor

@mymmrac mymmrac left a 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 🙁

@trim21
Copy link
Contributor

trim21 commented May 16, 2023

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.

@trim21
Copy link
Contributor

trim21 commented May 16, 2023

I didn't follow the development and discussion, so I'm a little confused by this thread.

why

We'd need to do some string parsing to get format the tasks and it will be more difficult to fetch task data (like aliases)

and use sed/read in fish completion to fetch alias?

I think it's already covered by task --list-all --silent, there is no need for sed and yaml parsing.

Did I miss something here?

  generate:
    desc: Runs Go generate to create mocks
    aliases: [gen, g]
$ task  --list-all --silent
clean
default
generate
gen
g
...

Is this referring to support variable completion in the future?

@danquah
Copy link
Contributor

danquah commented May 16, 2023

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 task generate:fixtures and then also update /completions or flip it around and use the compiled templates as the fixture. No matter what we should be aware that this introduces a new step in the release of Task.

With regards to the two other options I believe

  • Template only includes tool-level info (i.e. flags) and not task-level info (i.e. Task names). This means it can be called once on shell startup and we will fallback to task --list-all --silent for fetching tasks inside the completion script as we do currently. We'd need to do some string parsing to get format the tasks and it will be more difficult to fetch task data (like aliases)

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.

@pd93 pd93 added state: wip A work in progress. type: proposal A ticket that proposes a new feat/enhancement. labels May 31, 2023
@pd93 pd93 mentioned this pull request Nov 17, 2023
15 tasks
@pd93 pd93 added area: completions Changes related to shell completions and scripts. and removed state: wip A work in progress. type: proposal A ticket that proposes a new feat/enhancement. labels Jun 30, 2024
@pd93
Copy link
Member Author

pd93 commented Jun 30, 2024

This PR has been open way too long 😆

If merged this would currently break at least go-task/homebrew-tap but probably also other downstream packages

This comment made me pause for a bit and I never came back to it. Now that I've finally had some time to think about it again, I've reviewed all the existing work here and have decided to dramatically strip back the scope of this PR. I think I was trying to solve too many problems at once. What I've decided to do instead:

This will allow us to slowly migrate users and installers to use the new API without breaking anything today. In the future, when we are satisfied that nothing will break, we can then revisit how the scripts are generated without changing the API.

The API I have gone for is very similar to other modern CLI tools: task --completion <shell>. Most other CLIs would have a top-level command instead of a flag, but since Task's arguments are task names, this isn't possible.

I'd really appreciate feedback from anyone in this thread. Especially regarding the updated setup instructions since I'm not familiar with all shells (particularly Powershell). I've got it working nicely in fish and zsh so far 👍

@pd93 pd93 marked this pull request as ready for review June 30, 2024 02:11
notepad $profile
<TabItem value="2">
```shell
task --completion zsh > /usr/local/share/zsh/site-functions/_task

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it has to be global ?

Fish example is user based

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK (Please correct me if I'm wrong), there is no standard location for completion files within a user's $HOME directory in ZSH. The user is able to add paths to their $FPATH and then place their completions in that directory, but I figure that anyone doing this already knows what to do anyway.

I have no knowledge of Bash as I haven't used it in years, but I imagine the situation is similar.

The vast majority of users will want to eval/source the script in their dotfiles instead which is why I have marked the first method as "recommended". I actually considered not adding the second method at all.

completion.go Outdated Show resolved Hide resolved
"fmt"
)

//go:embed completion/bash/task.bash

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to define them in the methods? I mean does go:embed requires them to be defined in package scope.

I mean this constants will be defined in RAM even if you don't need completion, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go embeds must be defined at a package level as described in the docs.

It can only be used with variables at package scope, not with local variables

However this does not necessarily mean that the bytes are loaded into memory immediately. Even if they were to be loaded immediately, I'm not really worried about a few KB tbh.

Copy link
Member

@vmaerten vmaerten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good PR! Indeed, option 1 will be easier to use as it'll allow to get
updated automatically (and easier to put only one file in our config file instead of chmod + mv + etc)
This is fine for zsh and bash. I do not have knowledge about powershell nor fish
Good job on this 🚀

@pd93 pd93 enabled auto-merge (squash) September 2, 2024 19:18
@pd93 pd93 merged commit 3747b2a into main Sep 2, 2024
14 checks passed
@pd93 pd93 deleted the completions branch September 2, 2024 19:21
pd93 added a commit that referenced this pull request Sep 2, 2024
@ccxuy
Copy link

ccxuy commented Sep 4, 2024

which version contain this feature?

task --version
Task version: v3.38.0 (h1:O7kgA6BfwktXHPrheByQO46p3teKtRuq1EpGnFxNzbo=)

task --completion bash
.....
unknown flag: --completion
Failed at 177: task --completion bash

@vmaerten
Copy link
Member

vmaerten commented Sep 4, 2024

It's not released yet.
I'll be in the next release (and we do not have ETA for this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: completions Changes related to shell completions and scripts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add generate-completion command
8 participants