Before callback affects bash completions #1094

VirrageS · 2020-03-25T11:04:18Z

my urfave/cli version is

v1.22.2, v1.22.3

Checklist

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

Dependency Management

My project is using go modules.

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)
	}
}

Build the code above.
Enable completions
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

The text was updated successfully, but these errors were encountered:

coilysiren · 2020-03-31T00:09:22Z

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 · 2020-03-31T08:47:33Z

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.

coilysiren · 2020-03-31T21:47:12Z

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 · 2020-04-01T08:37:34Z

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 · 2020-04-02T08:06:27Z

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.

coilysiren · 2020-04-02T21:20:02Z

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 · 2020-04-04T10:48:02Z

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 · 2020-05-05T02:46:03Z

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 #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 · 2020-05-05T07:20:09Z

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 · 2020-05-22T07:55:48Z

Any decision on this one?

rliebz · 2020-05-23T00:37:58Z

@lynncyrin if you had any thoughts here

mjnovice · 2020-05-26T00:26:41Z

Agree with @VirrageS. Following the thread.

stale · 2020-08-24T00:37:51Z

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 · 2020-08-24T01:22:56Z

Any updates on this ?

stale · 2020-08-24T01:22:59Z

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 · 2020-11-22T03:04:00Z

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 · 2020-12-22T06:09:23Z

Closing this as it has become stale.

dbarrosop · 2022-07-21T11:42:33Z

Hello, probably related to this, if you have a flag that is required autocompletion for flags seems to break as well.

meatballhat · 2022-08-14T12:47:04Z

@dbarrosop Thank you for reporting this! Are you seeing this behavior with the latest 1.22.x release? I have no reason to believe that the bug has been fixed, but I'd like to make sure your report is in the right bucket.

VirrageS · 2022-08-14T17:46:20Z

@meatballhat it still reproduces on latest release.

dearchap · 2022-08-15T00:52:01Z

@VirrageS I dont think this is an easy fix. All the Before functions are called before the CompletionFunc all the way down the chain . So none of the Before functions should be executed during command completion.

VirrageS · 2022-08-16T13:27:57Z

Yup, that's sounds correct.

dearchap · 2022-08-18T01:25:04Z

Looks like the fix was simple after all. See PR #1457 . Can you test @VirrageS ?

VirrageS · 2022-08-19T18:49:52Z

@dearchap from what I see the PR targets v2. But the issue is raised for v1, which I currently use.

dearchap · 2022-08-19T22:44:36Z

@VirrageS See PR #1459

talarian1 · 2022-09-01T15:35:20Z

@meatballhat, @dearchap any planned release for v1 with a fix?
Thanks anyway.

dearchap · 2022-09-01T16:08:21Z

@talarian1 Its already merged to v1. Just waiting on @meatballhat to create a release tag. Thanks

meatballhat · 2022-09-05T14:14:43Z

@dearchap hey sorry, on it now!

meatballhat · 2022-09-05T14:16:43Z

Done? https://github.com/urfave/cli/releases/tag/v1.22.10

- Bash completion was rewritten from scratch as `.Before` is no longer called by github.com/urface/cli since v1.22.10 (see urfave/cli#1094). There might be a better way to do so, but with the additions of stacks finding all "service.***" in the current porject path seemed like a good start. - Add a `.` alias in arg parsing allowing a stack or a service to be acted upon when the user is currently in its directory.

- Bash completion was updated to init services itself as `.Before` is no longer called by github.com/urface/cli since v1.22.10 (see urfave/cli#1094). - Add a `.` alias in arg parsing allowing a stack or a service to be acted upon when the user is currently in its directory.

VirrageS added area/v1 relates to / is being considered for v1 kind/bug describes or fixes a bug status/triage maintainers still need to look into this labels Mar 25, 2020

stale bot added the status/stale stale due to the age of it's last update label Aug 24, 2020

stale bot removed the status/stale stale due to the age of it's last update label Aug 24, 2020

stale bot added the status/stale stale due to the age of it's last update label Nov 22, 2020

stale bot closed this as completed Dec 22, 2020

meatballhat reopened this Apr 22, 2022

meatballhat removed the status/stale stale due to the age of it's last update label Apr 22, 2022

meatballhat added this to the Release 1.22.8 milestone Apr 23, 2022

meatballhat changed the title ~~v1 bug: Before callback affects bash completions~~ Before callback affects bash completions Apr 23, 2022

meatballhat added help wanted please help if you can! and removed status/triage maintainers still need to look into this labels May 5, 2022

dearchap closed this as completed Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Before callback affects bash completions #1094

Before callback affects bash completions #1094

VirrageS commented Mar 25, 2020 •

edited by meatballhat

Loading

coilysiren commented Mar 31, 2020

VirrageS commented Mar 31, 2020

coilysiren commented Mar 31, 2020

VirrageS commented Apr 1, 2020 •

edited

Loading

VirrageS commented Apr 2, 2020

coilysiren commented Apr 2, 2020

VirrageS commented Apr 4, 2020 •

edited

Loading

rliebz commented May 5, 2020

VirrageS commented May 5, 2020

VirrageS commented May 22, 2020

rliebz commented May 23, 2020

mjnovice commented May 26, 2020

stale bot commented Aug 24, 2020

mjnovice commented Aug 24, 2020

stale bot commented Aug 24, 2020

stale bot commented Nov 22, 2020

stale bot commented Dec 22, 2020

dbarrosop commented Jul 21, 2022

meatballhat commented Aug 14, 2022

VirrageS commented Aug 14, 2022

dearchap commented Aug 15, 2022

VirrageS commented Aug 16, 2022

dearchap commented Aug 18, 2022

VirrageS commented Aug 19, 2022 •

edited

Loading

dearchap commented Aug 19, 2022

talarian1 commented Sep 1, 2022

dearchap commented Sep 1, 2022

meatballhat commented Sep 5, 2022

meatballhat commented Sep 5, 2022

Before callback affects bash completions #1094

Before callback affects bash completions #1094

Comments

VirrageS commented Mar 25, 2020 • edited by meatballhat Loading

my urfave/cli version is

Checklist

Dependency Management

Describe the bug

To reproduce

Observed behavior

Expected behavior

Additional context

Run go version and paste its output here

coilysiren commented Mar 31, 2020

VirrageS commented Mar 31, 2020

coilysiren commented Mar 31, 2020

VirrageS commented Apr 1, 2020 • edited Loading

VirrageS commented Apr 2, 2020

coilysiren commented Apr 2, 2020

VirrageS commented Apr 4, 2020 • edited Loading

rliebz commented May 5, 2020

VirrageS commented May 5, 2020

VirrageS commented May 22, 2020

rliebz commented May 23, 2020

mjnovice commented May 26, 2020

stale bot commented Aug 24, 2020

mjnovice commented Aug 24, 2020

stale bot commented Aug 24, 2020

stale bot commented Nov 22, 2020

stale bot commented Dec 22, 2020

dbarrosop commented Jul 21, 2022

meatballhat commented Aug 14, 2022

VirrageS commented Aug 14, 2022

dearchap commented Aug 15, 2022

VirrageS commented Aug 16, 2022

dearchap commented Aug 18, 2022

VirrageS commented Aug 19, 2022 • edited Loading

dearchap commented Aug 19, 2022

talarian1 commented Sep 1, 2022

dearchap commented Sep 1, 2022

meatballhat commented Sep 5, 2022

meatballhat commented Sep 5, 2022

VirrageS commented Mar 25, 2020 •

edited by meatballhat

Loading

Run `go version` and paste its output here

VirrageS commented Apr 1, 2020 •

edited

Loading

VirrageS commented Apr 4, 2020 •

edited

Loading

VirrageS commented Aug 19, 2022 •

edited

Loading