From 2139e763fbab20d8e64890065166d709fc0780a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 23 May 2022 20:23:42 +0200 Subject: [PATCH] Write Cobra deprecation messages to stderr We used to do the equivalent of `rootCmd.SetOut(os.Stdout)` because we thought that Cobra's "Out" stream represents standard output. However, upon closer inspection it turns out that this is Cobra's stream for usage errors and deprecation warnings, and those we want written to stderr as well. It is not clear to me why Cobra maintains a distinction between "Out" and "Err" streams since both seem to go to sdterr by default. This change also ceases our usage of `command.Print()` functions in favor of explicitly writing to `IOStreams.Out/ErrOut`. --- pkg/cmd/completion/completion_test.go | 2 +- pkg/cmd/gist/clone/clone_test.go | 2 +- pkg/cmd/pr/create/create_test.go | 2 +- pkg/cmd/repo/clone/clone_test.go | 4 ++-- pkg/cmd/root/help.go | 29 ++++++++++++++------------- pkg/cmd/root/help_topic.go | 26 +++++++++++++++--------- pkg/cmd/root/help_topic_test.go | 6 +++--- pkg/cmd/root/root.go | 20 +++++++++--------- 8 files changed, 51 insertions(+), 40 deletions(-) diff --git a/pkg/cmd/completion/completion_test.go b/pkg/cmd/completion/completion_test.go index 954a937be..d1b595544 100644 --- a/pkg/cmd/completion/completion_test.go +++ b/pkg/cmd/completion/completion_test.go @@ -54,7 +54,7 @@ func TestNewCmdCompletion(t *testing.T) { t.Fatalf("argument splitting error: %v", err) } rootCmd.SetArgs(argv) - rootCmd.SetOut(stdout) + rootCmd.SetOut(stderr) rootCmd.SetErr(stderr) _, err = rootCmd.ExecuteC() diff --git a/pkg/cmd/gist/clone/clone_test.go b/pkg/cmd/gist/clone/clone_test.go index 71773b0d2..ebf9b5c31 100644 --- a/pkg/cmd/gist/clone/clone_test.go +++ b/pkg/cmd/gist/clone/clone_test.go @@ -33,7 +33,7 @@ func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) cmd.SetArgs(argv) cmd.SetIn(stdin) - cmd.SetOut(stdout) + cmd.SetOut(stderr) cmd.SetErr(stderr) if err != nil { diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 9f7d8ce44..33feafa40 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -155,7 +155,7 @@ func TestNewCmdCreate(t *testing.T) { args, err := shlex.Split(tt.cli) require.NoError(t, err) cmd.SetArgs(args) - cmd.SetOut(stdout) + cmd.SetOut(stderr) cmd.SetErr(stderr) _, err = cmd.ExecuteC() if tt.wantsErr { diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index 06c53c325..8eefd32ca 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -74,7 +74,7 @@ func TestNewCmdClone(t *testing.T) { cmd.SetArgs(argv) cmd.SetIn(stdin) - cmd.SetOut(stdout) + cmd.SetOut(stderr) cmd.SetErr(stderr) _, err = cmd.ExecuteC() @@ -112,7 +112,7 @@ func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) cmd.SetArgs(argv) cmd.SetIn(stdin) - cmd.SetOut(stdout) + cmd.SetOut(stderr) cmd.SetErr(stderr) if err != nil { diff --git a/pkg/cmd/root/help.go b/pkg/cmd/root/help.go index d8b3e18b1..c7940faf0 100644 --- a/pkg/cmd/root/help.go +++ b/pkg/cmd/root/help.go @@ -3,6 +3,7 @@ package root import ( "bytes" "fmt" + "io" "sort" "strings" @@ -12,25 +13,25 @@ import ( "github.com/spf13/pflag" ) -func rootUsageFunc(command *cobra.Command) error { - command.Printf("Usage: %s", command.UseLine()) +func rootUsageFunc(w io.Writer, command *cobra.Command) error { + fmt.Fprintf(w, "Usage: %s", command.UseLine()) subcommands := command.Commands() if len(subcommands) > 0 { - command.Print("\n\nAvailable commands:\n") + fmt.Fprint(w, "\n\nAvailable commands:\n") for _, c := range subcommands { if c.Hidden { continue } - command.Printf(" %s\n", c.Name()) + fmt.Fprintf(w, " %s\n", c.Name()) } return nil } flagUsages := command.LocalFlags().FlagUsages() if flagUsages != "" { - command.Println("\n\nFlags:") - command.Print(text.Indent(dedent(flagUsages), " ")) + fmt.Fprintln(w, "\n\nFlags:") + fmt.Fprint(w, text.Indent(dedent(flagUsages), " ")) } return nil } @@ -52,8 +53,8 @@ func HasFailed() bool { // Display helpful error message in case subcommand name was mistyped. // This matches Cobra's behavior for root command, which Cobra // confusingly doesn't apply to nested commands. -func nestedSuggestFunc(command *cobra.Command, arg string) { - command.Printf("unknown command %q for %q\n", arg, command.CommandPath()) +func nestedSuggestFunc(w io.Writer, command *cobra.Command, arg string) { + fmt.Fprintf(w, "unknown command %q for %q\n", arg, command.CommandPath()) var candidates []string if arg == "help" { @@ -66,14 +67,14 @@ func nestedSuggestFunc(command *cobra.Command, arg string) { } if len(candidates) > 0 { - command.Print("\nDid you mean this?\n") + fmt.Fprint(w, "\nDid you mean this?\n") for _, c := range candidates { - command.Printf("\t%s\n", c) + fmt.Fprintf(w, "\t%s\n", c) } } - command.Print("\n") - _ = rootUsageFunc(command) + fmt.Fprint(w, "\n") + _ = rootUsageFunc(w, command) } func isRootCmd(command *cobra.Command) bool { @@ -84,7 +85,7 @@ func rootHelpFunc(f *cmdutil.Factory, command *cobra.Command, args []string) { cs := f.IOStreams.ColorScheme() if isRootCmd(command.Parent()) && len(args) >= 2 && args[1] != "--help" && args[1] != "-h" { - nestedSuggestFunc(command, args[1]) + nestedSuggestFunc(f.IOStreams.ErrOut, command, args[1]) hasFailed = true return } @@ -190,7 +191,7 @@ Read the manual at https://cli.github.com/manual`}) helpEntries = append(helpEntries, helpEntry{"FEEDBACK", command.Annotations["help:feedback"]}) } - out := command.OutOrStdout() + out := f.IOStreams.Out for _, e := range helpEntries { if e.Title != "" { // If there is a title, add indentation to each line in the body diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index 757e972ed..05798894b 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -1,7 +1,11 @@ package root import ( + "fmt" + "io" + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/text" "github.com/spf13/cobra" ) @@ -127,7 +131,7 @@ var HelpTopics = map[string]map[string]string{ }, } -func NewHelpTopic(topic string) *cobra.Command { +func NewHelpTopic(ios *iostreams.IOStreams, topic string) *cobra.Command { cmd := &cobra.Command{ Use: topic, Short: HelpTopics[topic]["short"], @@ -140,21 +144,25 @@ func NewHelpTopic(topic string) *cobra.Command { }, } - cmd.SetHelpFunc(helpTopicHelpFunc) - cmd.SetUsageFunc(helpTopicUsageFunc) + cmd.SetHelpFunc(func(c *cobra.Command, args []string) { + helpTopicHelpFunc(ios.Out, c, args) + }) + cmd.SetUsageFunc(func(c *cobra.Command) error { + return helpTopicUsageFunc(ios.ErrOut, c) + }) return cmd } -func helpTopicHelpFunc(command *cobra.Command, args []string) { - command.Print(command.Long) +func helpTopicHelpFunc(w io.Writer, command *cobra.Command, args []string) { + fmt.Fprint(w, command.Long) if command.Example != "" { - command.Printf("\n\nEXAMPLES\n") - command.Print(text.Indent(command.Example, " ")) + fmt.Fprintf(w, "\n\nEXAMPLES\n") + fmt.Fprint(w, text.Indent(command.Example, " ")) } } -func helpTopicUsageFunc(command *cobra.Command) error { - command.Printf("Usage: gh help %s", command.Use) +func helpTopicUsageFunc(w io.Writer, command *cobra.Command) error { + fmt.Fprintf(w, "Usage: gh help %s", command.Use) return nil } diff --git a/pkg/cmd/root/help_topic_test.go b/pkg/cmd/root/help_topic_test.go index c5c51408f..5f6825912 100644 --- a/pkg/cmd/root/help_topic_test.go +++ b/pkg/cmd/root/help_topic_test.go @@ -61,11 +61,11 @@ func TestNewHelpTopic(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, _, stdout, stderr := iostreams.Test() + ios, _, _, stderr := iostreams.Test() - cmd := NewHelpTopic(tt.topic) + cmd := NewHelpTopic(ios, tt.topic) cmd.SetArgs(append(tt.args, tt.flags...)) - cmd.SetOut(stdout) + cmd.SetOut(stderr) cmd.SetErr(stderr) _, err := cmd.ExecuteC() diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 4c21330a5..4aa65c798 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -56,14 +56,16 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { }, } - cmd.SetOut(f.IOStreams.Out) - cmd.SetErr(f.IOStreams.ErrOut) + cmd.SetOut(f.IOStreams.ErrOut) // command usage summary and deprecation warnings + cmd.SetErr(f.IOStreams.ErrOut) // error messages cmd.PersistentFlags().Bool("help", false, "Show help for command") - cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) { - rootHelpFunc(f, cmd, args) + cmd.SetHelpFunc(func(c *cobra.Command, args []string) { + rootHelpFunc(f, c, args) + }) + cmd.SetUsageFunc(func(c *cobra.Command) error { + return rootUsageFunc(f.IOStreams.ErrOut, c) }) - cmd.SetUsageFunc(rootUsageFunc) cmd.SetFlagErrorFunc(rootFlagErrorFunc) formattedVersion := versionCmd.Format(version, buildDate) @@ -108,10 +110,10 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.AddCommand(labelCmd.NewCmdLabel(&repoResolvingCmdFactory)) // Help topics - cmd.AddCommand(NewHelpTopic("environment")) - cmd.AddCommand(NewHelpTopic("formatting")) - cmd.AddCommand(NewHelpTopic("mintty")) - referenceCmd := NewHelpTopic("reference") + cmd.AddCommand(NewHelpTopic(f.IOStreams, "environment")) + cmd.AddCommand(NewHelpTopic(f.IOStreams, "formatting")) + cmd.AddCommand(NewHelpTopic(f.IOStreams, "mintty")) + referenceCmd := NewHelpTopic(f.IOStreams, "reference") referenceCmd.SetHelpFunc(referenceHelpFn(f.IOStreams)) cmd.AddCommand(referenceCmd)