go-task / task

A task runner / simpler Make alternative written in Go
https://taskfile.dev
MIT License
11.35k stars 613 forks source link

Add generate-completion command #293

Closed Chi-teck closed 1 month ago

Chi-teck commented 4 years ago

This is follow-up for #103.

Example: task generate-completion --bash >> ~/.bashrc

seebi commented 2 years ago

I would like to emphasise the importance of this issue :-)

Not having a first-class citizen completion setup for go-task is a major adoption blocker in my POV.

I hope others will jump in here.

FYI: The great Click command line python framework provides completion support for its apps like this: https://click.palletsprojects.com/en/8.0.x/shell-completion/ (just as an info, how this could be done) - is there something similar for go?

BTW: sawadashota's go-task-completions.git project is ok but is never able to be great, at least not without support from go-task itself. Things missing in this project:

wyuenho commented 2 years ago

I agree. Task's completion is not nearly on-par with gnumake.

andreynering commented 2 years ago

I'd love to get some help from the community on this one.

andreynering commented 2 years ago

And to be clear, we already have completion scripts here: https://github.com/go-task/task/tree/master/completion

We just don't have a --generate-completion flag (or similar name).

pimjansen commented 2 years ago

@andreynering how to load that into ZSH?

drorata commented 2 years ago

Adding on @pimjansen, I simply tried to add https://github.com/go-task/task/blob/master/completion/zsh/_task to my .zshrc but it didn't work. The error I got is:

_arguments:comparguments:325: can only be called from completion function

@andreynering any idea?

Jeremie-Chauvel commented 2 years ago

Hi everyone, I just found out that task has indeed an available completion, I highly recommend adding a few words regarding this in the documentation (in the installation section maybe ?), I can do it if you like.

For @drorata, @pimjansen or other people trying to install the zsh completion, basically you need to download the given completion, make it executable and move it to a directory in fpath (list of directory containing completion fonction) or declare it yourself as completion for task

curl https://raw.githubusercontent.com/go-task/task/master/completion/zsh/_task -o _task
chmod +x _task

then either move it to a fpath directory

echo $fpath

choose a directory and then

cp _task <my_directory_in_fpath_I_might_need_sudo_depending_on_directory>

or just declare the completion in .zshrc:

compdef _task task
Jeremie-Chauvel commented 2 years ago

Hi @andreynering, I don't really know golang, however I'm interested in this feature, and would love to submit a pull request to implement it. (As well as contributing to the documentation to advertise the already available completion).

Would a pull request be welcome ?

andreynering commented 2 years ago

Hi @Jeremie-Chauvel,

Contribution to documentation would be welcome. Perhaps this would be a new section in the Installation page.

Regarding code contribution, that's welcome, too. I'm still not 100% on how that is supposed to work, though. I'm not too familiar to completion scripts, they have 100% been made by contributions from the community, and I don't even use it myself.

Seems that --generate-completion is a convention. Perhaps we could look for a library this generate those for us?

Jeremie-Chauvel commented 2 years ago

Sure, for the --generate-completion flag, I propose looking into other implementation first to have a strategy you can understand and challenge before moving to implementation ☺️ First that comes to mind is GitHub CLI

Jeremie-Chauvel commented 2 years ago

From a quick look into github-cli, it seems the projet is relying on cobra to build there CLI interface. One of Cobra feature is generating the completion for the built CLI interface example with github CLI.

Rebuilding Task CLI interface seems a bit overkill, so I will look into more solutions/implementation.

Jeremie-Chauvel commented 2 years ago

Hi @andreynering, I worked with @viconnex to document the existing completions.

I will tackle the flag in coming weeks

andreynering commented 2 years ago

@Jeremie-Chauvel #906 is merged. Thanks!

Jeremie-Chauvel commented 1 year ago

Hi @andreynering,

It took some to get get back to you but from a small investigation, I propose three ways of addressing this issue:

Solution Ease of implementation completions maintenance impact on CLI size/dependencies
Switch to Cobra quite hard as it requires a rewrite of the complete CLI interface none as completions are generated and always up to date new dependency for go-task
Embark current completions pretty easy as we just copy completions when building a release completions are still written by hand and can be out of date minor impact without a new (code) dependency
completion generation with complete medium as we need to copy the whole interface to discribe it in complete generated (not for powershell) but still need to be kept up to date

Knowing this I would recommend going with solution 1: refactoring to Cobra, as it would mean no further work needed on completions. However, should it not be your vision/conclusion, I'm of course open to implementing solution 2, 3 or something else you might think of.

Thanks for your help.

andreynering commented 1 year ago

Hi @Jeremie-Chauvel,

I'm totally fine with migrating to Cobra if that bring improvements.

I had initially choosen pflag because it's more minimalistic. I now noticed Cobra uses pflag under the hood, that may even make migration somewhat easier. 🙂

Having completions being automatically generated would be really nice, indeed.

Jeremie-Chauvel commented 1 year ago

Hi @andreynering,

I'm very sorry for the delay, I found some time today to give migrating task to cobra a go, I have a simple migration that 'seems' to work, my next steps are implementing the CLI interface in a more cobra idomatic way.

On my way, I'm looking for tests I can run to validate the interface migration, I found the tests in CI (that I can run against my cobra implementation but those doesn't seem to target the CLI interface), do you have a recommandation, something I missed or should I write such tests myself?

andreynering commented 1 year ago

Hi @Jeremie-Chauvel,

We indeed don't have tests for the CLI interface currently. That's a bit hard to test and IMO it's not strictly needed.

That said, if you happen to find a way to easily test the Cobra definition without having to call the binary in tests, that's certainly a nice addition to the project. If that shows too hacky, though, it's fine to skip IMO.

I can certainly help you test it manually. It's just a matter of opening the PR when you're ready.

And you don't need to be sorry, we're all busy. Thanks for your work on this, it's very welcome! 🙂

gedw99 commented 1 year ago

Would be really nice to have this feature.

for zsh you need to do this, but the command can automate this for you.

mv path/to/_task /usr/local/share/zsh/site-functions/_task

autoload -U compinit
compinit -i

Would not we just need to embed the _task at build time, and then the generate-completion command does the rest ?

danquah commented 1 year ago

I'll be happy to help with moving this along. I could do one of the following.

  1. Embedding the completion-files into task and having it echo it would work as an interim solution, and might be a quick way to get something more stable up and running that stays more in sync with the codebase. It would require us to keep the files up to date, but as they mostly relies on calling --list-all it should be ok.
  2. @Jeremie-Chauvel Depending on how you fared I could either take a fresh stab at converting to Cobra, or I could lend a helping and if you are stuck somewhere? I'm no expect in Cobra, but I do have som some consistent time to put in so I'm pretty sure I can get most things to work over time.
Jeremie-Chauvel commented 1 year ago

Hi @danquah,

Don't have my computer on me, however I remember that I blocked on an architecture issue where Cobra needs to infer the possible params before launching the main program. However doing so required quite some work to make current task file inferring output Cobra compatible and I wasn't super confident that it would work with autocomplete script generation as it is dynamic. Would probably need to POC the second part to ensure feasibility.

I can push you my draft tomorrow if you like and do agree that pushing embedded autocomplete files would be a nice quick win.

gedw99 commented 1 year ago

Glad it’s moving forward

sorry @Jeremie-Chauvel i could not help . No time !

danquah commented 1 year ago

Sounds great @Jeremie-Chauvel - and no stress, just make it available when you have time and I'll help with whatever is needed.

Jeremie-Chauvel commented 1 year ago

Hi, I pushed my current work on https://github.com/Jeremie-Chauvel/task/tree/switch-interface-to-cobra, I took the liberty to rebase it on the main branch but I didn't review the general changes. Current issue is as I said executing the task listing in cobra init function (L126 in my POC). Since I have no golang knowledge I was a bit stuck and lacked the time to dive in :confused:

I do believe the quick win solution is more pragmatic for now even if being able to generate those autocompletion would be amazing!

danquah commented 1 year ago

I'll give it a whirl and report back :)

danquah commented 1 year ago

I'm still on this - had to wrap my head around what we're trying to do here but I think I am on track to having something working within a week now. There has been made some minor changes in task.go since you started your work @Jeremie-Chauvel (spottet https://github.com/go-task/task/commit/8b72c86ba5b81075ba120284066df9d736ebd94f so far) so I'll see whether I'll start from scratch heavily inspired by your good work (looks like you where pretty close to have it working!), or whether I can just backport the changes.

I'll be back within a week or so.

danquah commented 1 year ago

Have some stuff working now

$ go run cmd/cobra/task.go clean
task: [clean] rm -rf dist/
task: [clean] rm -rf tmp/

We should probably be aware that Cobra does not support the concept of invoking multiple commands the way task does. In Cobra-speak multiple commands becomes a call to a single sub-command like eg kubectl get pods. In this case pods is the sub-command, and while get is also handled by calling some post-hooks, in the end pods is the command that is being invoked (as I understand it).

Now, @Jeremie-Chauvel has done some clever stuff to start off with a root-command during init that gets all the arguments, and then dynamically set up commands at the same level - in other words, as Cobra starts up it only sees the one root command, but when that command gets running, Cobra now knows of all the tasks specified in the taskfile. This way the root command could for instance use Cobra to list tasks, or - as is our goal - generate completions. We're never using Cobra for running the actual command/task - we're only piggybacking on its code for usage/help/completions etc.

So, we're making it work. But, at this point I can't really tell whether this is a reasonable way to use just a part of Cobra

There is an issue on the whole concept of supporting multiple commands spf13/cobra: https://github.com/spf13/cobra/issues/726 - the gist of the issue is that it is not currently something that is going to be supported.

I'll hack a bit more on this and get something stable. I'll probably push something tomorrow or the day after.

But then we'll have to decide whether we want to continue down this path - and if so, it is only to be used for completions - or whether things like usage, task listing and help for a specific task should also be handled by the Cobra code.

Jeremie-Chauvel commented 1 year ago

Great summary and work @danquah!

Yeah, that was my concern as well, I don't remember if Cobra is dependency free or if we could build on top of the completion primitive directly 🤔

danquah commented 1 year ago

If we could build on top of the completion primitive directly

Might be possible. The bigger questions is probably whether we want to use other stuff from Cobra. On the one hand offloading things such as usage, help and parameter/argument validation/handling to Cobra would let task focus on the core problems of running the tasks. On the other hand, it might constrain what is possible to configure for a given task - eg, if we want some really special handling of eg. arguments.

pd93 commented 1 year ago

I'm going to apologise for the essay in advance - I've been holding back on these thoughts for a little while now :sweat_smile:


My 2 cents: I've used Cobra a few times on other projects and I've always had a positive experience, but I'm not convinced that Cobra is the right solution here. I held off commenting previously as its been a while since I used it and wasn't sure if things might have changed. However, it seems that you're now running into some of the problems that I was concerned about.

Task does not really behave like a "regular" CLI. We don't have an argument hierarchy (which is what Cobra does well). Instead, all our arguments are just tasks and everything else (including commands) are flags. This is so that we don't need to reserve task names and can add commands without breaking Taskfiles. It also means that users can run tasks without the need to add an additional command namespace (like task run <mycmd>). This works well for us, but causes headaches in Cobra. It can probably be made to work, but is this really what we want?

Shoe-horning a large dependency that doesn't fit so that we can use a relatively small feature (completions) feels wrong to me.

offloading things such as usage, help and parameter/argument validation/handling to Cobra

usage/help is actually already handled in Task by pflag (which is a Cobra dependency), so there would be no change here. As for validation, I personally think Task having control over this is better. It gives us more control over the messages/format and things like exit codes.

if we could build on top of the completion primitive directly

Cobra does not expose its code for completion generation and is heavily dependant on the data structure that Cobra uses. Unfortunately, I don't think using this separately is an option.


Well this got a bit downbeat :/ So I'm going to write down a few things that I think would go a long way to solving the problem without Cobra. First, let's review the actual issues we have today as discussed in this (and previous) threads. I think it boils down to these three things:

  1. Our completion scripts are often out-of-date with Task
  2. Our completion scripts are complex so no-one updates them
  3. There is no first-class way to setup/install completions using Task (and keep them updated)

Our current completions piggyback our --list-all command. The output of this is human readable and not ideal for parsing by shells. This leads to complex completion scripts that break whenever we update the output of the --list-all flag. We need a better way to pass data to the completion scripts that is machine readable.

My initial thought was "add another flag", for the scripts to call that outputs machine readable information about task, but this has its own issues:

Instead of constructing the script in a shell language, a nicer way might be to construct the script in Golang via a set of templates. This has the advantage of not exposing any new flags to the CLI and allows us to pass any set of variables we like into the template without worrying about splitting strings or parsing JSON. We can loop over flags/args and directly inject the data into the template. As a bonus, templates can be embedded into the binary and all of this is a part of the standard library.

This solves all three issues:

The only problem with this approach is that it doesn't really facilitate anyone wanting to write a custom completion script. However, this is likely a tiny number of users and realistically, if we make our completion scripts good and simple to install. Why would they need to?

andreynering commented 1 year ago

Seems that Cobra itself use Go templates to generate the completion scripts?

We could in theory use the same scripts (or use them as inspiration).

I'd just suggest having the scripts as standalone files, though. We can embed the files using Go embed then.

If we decide to keep the generated ones in the repository (and include in releases) we can have a script to generate each and commit in the right directory.

danquah commented 1 year ago

I think this would make perfect sense! With regards to auto-generating we could also check whether the file is up to date via a github action and just fail a PR if a diff is detected.

I have no background in creating shell completion scripts ... but, it should be possible to grok how they work.

pd93 commented 1 year ago

@andreynering Agreed with all your comments. Drafted #1157 so you can see what this might look like.

With the way I've done it, I don't see any particular need to store the compiled templates unless I'm missing something?

@danquah I also don't have a huge amount of experience with completion scripts (besides tinkering with a few ZSH bits for my own dotfiles), so I'll appreciate any support from the community I can get 😄

danquah commented 1 year ago

Ok. I'm switching my focus to your suggestion @pd93. Should we need it in the future for reference I've pushed my last revision of the other approach here.

So as I see it we have the following work in front of us (feel free to expand the list)

With regards to storing the compiled templates or not, I'm currently for doing both: I'll run through the current installation methods tomorrow but as things looks right now, we have some package managers that installs completions alongside the binary (eg. homebrew) and others that currently only installs the binary (eg. fedora). So, if we switch to new and improved completions and only deliver these via task --completion we would have to visit each of the packages we currently have that delivers completion and do something. I'm sure we could switch them to use task --completion but it would be extra work we would have to do in order to complete our work.

There is also an argument for the simplicity and performance of just having to source a single file at shell startup. But, in my eyes thats less of an argument than the whole "we have to update a bunch of packages".

One downside is that we will have to keep the compiled templates in the repository in sync - but as mentioned earlier we could probably fix that via pull-request gate that fails the PR if the compiled templates drifts out of sync.

So, all in all I suggest we make sure to keep the completions/ and ensure it is in sync with the codebase to avoid breaking compatibility.

pd93 commented 1 year ago

With regards to storing the compiled templates or not

@danquah The problem with storing the compiled template is that we'd be passing in Taskfile-specific arguments at compile time, so the compiled file is not going to be reusable. i.e. it will only work for one Taskfile and will not update when the user changes something.

The advantage of calling task --completion every time the user hits TAB is that it will always be up to date. Even if you change the Taskfile or install a different version of Task. I think that if we are going to use templates, this is the only (good) way.

Take a look at how the Hugo docs (which uses Cobra/pflag) asks their users to install completions:

I think that it is fairly easy to do this during installation - and if not, its fairly easy for the user to add source <(task --completion bash) to their .bashrc.

gedw99 commented 1 year ago

Probably with supporting aqua. It’s like nix but works on windows too. It’s all golang …

https://aquaproj.github.io/

danquah commented 1 year ago

@pd93

The problem with storing the compiled template is that we'd be passing in Taskfile-specific arguments at compile time, so the compiled file is not going to be reusable

Ah, I think we might be on the same page. I always assumed the completion-file only contained the logic that is the same for all taskfiles, and would have to call out to task whenever it needs to to the actual completion. So, things like options could be hardcoded into the completion-file as they only change with upgrades, but when it comes to completing the string "bui" the completion file would still have to call task --list-all to figure out that we have a task called build.

Take a look at how the Hugo docs (which uses Cobra/pflag) asks their users to install completions:

Agree - that's the way to do it. By allowing the user to do hugo completion bash > /etc/bash_completion.d/hugo they avoid calling hugo at every launch of the shell, but instead wait until the user actually tries to complete something related to hugo.

An related thought that is probably a dangerous detour: I guess we could make our lives easier by adding some secret parameter/command to task that does the completion and lets us avoid having to re-implement the same logic in x different shells....

danquah commented 1 year ago

@pd93 I like the idea of getting the setup for the completion command merged, and then start implementing the scripts for the various shells.

How about starting off with having the --completion output the current completion files? We could literally just drop the current files into the templates directory. We could then convert them to "proper" templates as we go along - first step could be to get the flags generated dynamically. This would decouple the task of implementing the features from updating documentation / packages / etc to use --completion

Did a quick first take based off of your work here (diff).

I'll have to dig a bit deeper into how the completions work (goal for next week) - but from the existing completions I'm getting the feeling that it would not be a problem to keep compiled version of the completions in the repo for backwards compatibility and the situations where you do not want to call a command in your .rc file. Any further thoughts on this will be most welcomed.

pd93 commented 1 year ago

@danquah I've pushed an update to #1157 and added a comment. Let's continue the implementation-specific conversation there.

pd93 commented 4 months ago

For anyone following this. Some progress here: https://github.com/go-task/task/pull/1157#issuecomment-2198405718