spf13 / cobra

A Commander for modern Go CLI interactions
https://cobra.dev
Apache License 2.0
37.8k stars 2.83k forks source link

completion and __complete commands should always use stdout #2186

Closed jtackaberry closed 3 weeks ago

jtackaberry commented 4 weeks ago

I'm calling rootCmd.SetOut(os.Stderr) in my program because stdout is intended for specific purpose (redirection or piping), so Cobra output needs to be sent to stderr.

The key exception is completion. Completion wrappers invoke the app with completion or __complete commands and these are intended to be outputted to stdout. So calling SetOut(os.Stderr) on the root command breaks this.

It seems to me there's no use case in which completion or __complete should not write to stdout? In which case, shouldn't this and this force os.Stdout?

I am able to workaround completion by defining my own custom completion cobra.Command where I can force it to stdout, but near as I can tell __complete isn't accessible to me. Is a workaround possible, or does this need to be fixed in Cobra's internals?

Happy to submit a PR.

marckhouzam commented 4 weeks ago

Thanks @jtackaberry. Having the ability to change the output stream for the __complete command is useful to be able to run unit tests for that command and check its output.

What I suggest in your case, is that before you call rootCmd.SetOut() you check if you are dealing with the __complete command and make an exception in that case. You should be able to use rootCmd.Find(os.Args[1:]) to perform that check.

jtackaberry commented 3 weeks ago

What I suggest in your case, is that before you call rootCmd.SetOut() you check if you are dealing with the __complete command and make an exception in that case. You should be able to use rootCmd.Find(os.Args[1:]) to perform that check.

Thanks very much @marckhouzam for the suggested workaround.

For future Googlers who want a lazyweb copy-paste answer, here it is:

completionCommands := []string{"completion", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd}
if len(os.Args) < 2 || !slices.Contains(completionCommands, os.Args[1]) {
    rootCmd.SetOut(os.Stderr)
}

Edit: revised based on continued discussion below.

marckhouzam commented 3 weeks ago

Glad it works. It may be safer to do something like

cmd, _, _ := rootCmd.Find(os.Args[1:])
if cmd != cobra.ShellCompRequestCmd {
   rootCmd.SetOut(os.Stderr)
}
jtackaberry commented 3 weeks ago

cmd.Name() I guess you mean? Since cmd is a *cobra.Command and cobra.ShellCompRequestCmd is a string.

~But AFAICT rootCmd.Find() always returns rootCmd as the cmd return value. Which, now that I think of it, seems like a bug?~

jtackaberry commented 3 weeks ago

Apologies, disregard, my brain has begun the weekend ahead of my fingers.

jtackaberry commented 3 weeks ago

@marckhouzam ah, ok, I think I found the source of my confusion.

This works ok:

cmd, _, _ := rootCmd.Find([]string{"completion"})

cmd here is indeed the completion command.

However with this:

cmd, _, _ := rootCmd.Find([]string{"__complete"})

cmd is just rootCmd. I believe this is because the __complete command isn't created until rootCmd.Execute() is called, but I need to do the test and SetOutput() before Execute().

Ultimately what I was doing before was a very indirect and pointless way of checking os.Args for the complete command. This isn't as robust as using Cobra's arg parsing, but it's probably good enough as a workaround since the invocation of complete is under our control (seeing as it is the product of the output of the completion command).

Edit: actually the completion command works only because I'm explicitly adding my own such command. If relying on the default built-in completion command, rootcmd.Find([]string{"completion"}) prior to Execute() would fail to find that as well.

marckhouzam commented 3 weeks ago

Sorry @jtackaberry , you are correct. You can’t call find () for the __complete command before Execute() is run. Your way is the right way