Cli: v1 bug: Before callback affects bash completions

[VirrageS]

my urfave/cli version is

v1.22.2, v1.22.3

Checklist

• [x] Are you running the latest v1 release? The list of releases is here.
• [x] Did you check the manual for your release? The v1 manual is here
• [x] Did you perform a search about this problem? Here's the Github guide about searching.

Dependency Management

• [x] My project is using go modules.
• [ ] My project is using vendoring.
• [ ] My project is automatically downloading the latest version.
• [ ] I am unsure of what my dependency management setup is.

Describe the bug

I wanted to introduce pre-check (with .Before callback) that would be run if the command is being executed. But it seems like the pre-check is also executed when I want to only show the completions for a given command what breaks the completions output if the .Before has printed anything (in particular the error).

To reproduce

package main

import (
    "errors"
    "fmt"
    "log"
    "os"

    "github.com/urfave/cli"
)

func main() {
    app := cli.NewApp()
    app.Name = "example"
    app.EnableBashCompletion = true
    app.Before = func(_ *cli.Context) error { return errors.New("uups") }
    app.Commands = []cli.Command{{
        Name:   "cmd",
        Action: func(c *cli.Context) error { return nil },
        Flags:  []cli.Flag{cli.StringFlag{Name: "abc"}},
        BashComplete: func(c *cli.Context) {
            fmt.Println("--abc")
        },
    }}
    app.Action = func(c *cli.Context) error {
        fmt.Println("Hello friend!")
        return nil
    }

    err := app.Run(os.Args)
    if err != nil {
        log.Fatal(err)
    }
}

1. Build the code above.
2. Enable completions
3. example cmd [TAB]

Observed behavior

$ example cmd ......2020/03/25 11:53:14 uups
$ example cmd
GLOBAL OPTIONS  COMMANDS  USAGE  NAME  --
                                                                                example - A new cli application                                           uups
   --help, -h  show help                                                        example [global options] command [command options] [arguments...]
   cmd                                                                          help, h  Shows a list of commands or help for one command

Expected behavior

Show completions for the given cmd:

$ example cmd [TAB]
--abc

Additional context

Looking at the code I've noticed that the commands are parsed/visited hierarchically and this also applies for the completions. But I'm not sure if the order should be as it is now. To me completions should be executed first even if the command is nested.

Run go version and paste its output here

go version go1.14 darwin/amd64

[lynncyrin]

I wanted to introduce pre-check (with .Before callback) that would be run if the command is being executed. But it seems like the pre-check is also executed when I want to only show the completions for a given command what breaks the completions output if the .Before has printed anything (in particular the error).

Is the idea that .Before shouldn't be run when you're doing tab completion?

[VirrageS]

Is the idea that .Before shouldn't be run when you're doing tab completion?

Hm, I think it all comes down to this. Yes.

And I think the .After as well.

[lynncyrin]

Interesting! Do you think the most of the people using that package have that same idea? I ask because if they don't, then this would be a breaking change for them.

[VirrageS]

I would say yes. As provided in the example, the default behaviour of returning an error in .Before breaks the .BashComplete output. So then what is the point of returning an error in .Before if it breaks some other functionality.

Another problem would be printing anything in the .Before - it cannot be done without buffering the output and then printing it once the .Action (not .BashComplete) is executed.

I'm probably missing someone as I have mostly my own use-cases in mind but it seems like using .Before and .After together with .BashComplete is not possible without super crazy hacks. .Before and .After seem to be strictly connected to the .Action not the .BashComplete which lives its own life.

To mitigate the breaking change (if anyone would be affected) new callbacks could be created: .(Before|After)BashComplete. But personally I don't see any use case for them.

[VirrageS]

I could write a fix for that but I would need to know the decision: if we want to do it and if the approach of "skipping .Before and .After if bash completing" is okay.

[lynncyrin]

if we want to do it and if the approach of "skipping .Before and .After if bash completing" is okay

I would need to think about this a lot more before I can make this decision!

Also cc @urfave/cli, in case anyone else has a more solid point of view here 🙏

[VirrageS]

To support my arguments here is my use-case:

I use CLI to communicate with some server via API. To make sure that user correctly connected to the server I do pre-check which sends ping message to server. If that succeeds I set up the CLI and run the command, if not I print the error message. That works well but the problem is that if someone wants to do cli [TAB] and the configuration is not correct, then the pre-check will fail. And instead of autocompletion the user will get an error message as autocomplete and that's not user-friendly (as shown in the example). Then I thought that I can use .Before on the root command to do pre-check so only when actual command is requested (not autocomplete) I will display the error message. But to my surprise it wasn't working as suspected and thus the issue.

Also I was kind of surprised that .Before prints error together with the usage template if the error is returned (I would suspect some control over it) but I think I can live with that.

[rliebz]

Also I was kind of surprised that .Before prints error together with the usage template if the error is returned

Good call there. As of https://github.com/urfave/cli/pull/1117 that is no longer the case.

We don't put any documentation around what the intended purpose of Before and After are, so it's hard to say what people expect to happen there. In terms of completion, my guess is most people either ignore it, or enable it and don't think about it unless it breaks. I will say, it's unlikely that any printing to stdout or stderr during completion is correct, including printing errors.

I'd have two questions:

• Would the behavior of a BeforeFunc ever influence what's available in a completion?
• Is it reasonable for users to program a separate code path in a BeforeFunc to behave differently during completion?

My guess to both of those are "no" and "no", and we'd be in a better spot if we didn't run Before if we were completing.

I will also note that I have not used a BeforeFunc in my apps, and I have heavily customized my completion behavior for various reasons, so I'm likely not generally representative of the average user here.

[VirrageS]

Good call there. As of #1117 that is no longer the case.

Sounds awesome :)

My guess to both of those are "no" and "no", and we'd be in a better spot if we didn't run Before if we were completing.

That's my view as well. Before/After are kind of separate from completions and in my opinion shouldn't be mixed as it only leads to problems or nasty hacks. If we really would like to have something like that for completions as well, I would just suggest creating new callbacks: Before/AfterCompletions.

[VirrageS]

Any decision on this one?

[rliebz]

@lynncyrin if you had any thoughts here

[mjnovice]

Agree with @VirrageS. Following the thread.

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[mjnovice]

Any updates on this ?

[stale[bot]]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale[bot]]

Closing this as it has become stale.