From c52fc72291f01d4946ed9af5263487ec54ab0be1 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 25 Sep 2020 15:44:34 +0200 Subject: [PATCH 1/6] Extract bareHTTPClient function --- pkg/cmd/root/root.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 334ee418d..8f0b45817 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -95,13 +95,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { // the `api` command should not inherit any extra HTTP headers bareHTTPCmdFactory := *f - bareHTTPCmdFactory.HttpClient = func() (*http.Client, error) { - cfg, err := bareHTTPCmdFactory.Config() - if err != nil { - return nil, err - } - return factory.NewHTTPClient(bareHTTPCmdFactory.IOStreams, cfg, version, false), nil - } + bareHTTPCmdFactory.HttpClient = bareHTTPClient(f, version) cmd.AddCommand(apiCmd.NewCmdApi(&bareHTTPCmdFactory, nil)) @@ -117,6 +111,16 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { return cmd } +func bareHTTPClient(f *cmdutil.Factory, version string) func() (*http.Client, error) { + return func() (*http.Client, error) { + cfg, err := f.Config() + if err != nil { + return nil, err + } + return factory.NewHTTPClient(f.IOStreams, cfg, version, false), nil + } +} + func resolvedBaseRepo(f *cmdutil.Factory) func() (ghrepo.Interface, error) { return func() (ghrepo.Interface, error) { httpClient, err := f.HttpClient() From f00a0f2854f6aa0ca96a2a8e3bbe36f7ea5cf300 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 25 Sep 2020 15:55:28 +0200 Subject: [PATCH 2/6] Move completion command out of root package --- pkg/cmd/{root => completion}/completion.go | 6 +++--- pkg/cmd/{root => completion}/completion_test.go | 8 ++++++-- pkg/cmd/root/root.go | 3 ++- 3 files changed, 11 insertions(+), 6 deletions(-) rename pkg/cmd/{root => completion}/completion.go (93%) rename pkg/cmd/{root => completion}/completion_test.go (92%) diff --git a/pkg/cmd/root/completion.go b/pkg/cmd/completion/completion.go similarity index 93% rename from pkg/cmd/root/completion.go rename to pkg/cmd/completion/completion.go index a96b69840..7c1ec46f0 100644 --- a/pkg/cmd/root/completion.go +++ b/pkg/cmd/completion/completion.go @@ -1,4 +1,4 @@ -package root +package completion import ( "errors" @@ -6,11 +6,11 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/pkg/cmdutil" - "github.com/cli/cli/pkg/iostreams" "github.com/spf13/cobra" ) -func NewCmdCompletion(io *iostreams.IOStreams) *cobra.Command { +func NewCmdCompletion(f *cmdutil.Factory) *cobra.Command { + io := f.IOStreams var shellType string cmd := &cobra.Command{ diff --git a/pkg/cmd/root/completion_test.go b/pkg/cmd/completion/completion_test.go similarity index 92% rename from pkg/cmd/root/completion_test.go rename to pkg/cmd/completion/completion_test.go index 505a65319..d75c71398 100644 --- a/pkg/cmd/root/completion_test.go +++ b/pkg/cmd/completion/completion_test.go @@ -1,9 +1,10 @@ -package root +package completion import ( "strings" "testing" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/google/shlex" "github.com/spf13/cobra" @@ -45,7 +46,10 @@ func TestNewCmdCompletion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { io, _, stdout, stderr := iostreams.Test() - completeCmd := NewCmdCompletion(io) + f := &cmdutil.Factory{ + IOStreams: io, + } + completeCmd := NewCmdCompletion(f) rootCmd := &cobra.Command{Use: "gh"} rootCmd.AddCommand(completeCmd) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 8f0b45817..8549ac3bc 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -13,6 +13,7 @@ import ( aliasCmd "github.com/cli/cli/pkg/cmd/alias" apiCmd "github.com/cli/cli/pkg/cmd/api" authCmd "github.com/cli/cli/pkg/cmd/auth" + completionCmd "github.com/cli/cli/pkg/cmd/completion" configCmd "github.com/cli/cli/pkg/cmd/config" "github.com/cli/cli/pkg/cmd/factory" gistCmd "github.com/cli/cli/pkg/cmd/gist" @@ -88,7 +89,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.AddCommand(configCmd.NewCmdConfig(f)) cmd.AddCommand(creditsCmd.NewCmdCredits(f, nil)) cmd.AddCommand(gistCmd.NewCmdGist(f)) - cmd.AddCommand(NewCmdCompletion(f.IOStreams)) + cmd.AddCommand(completionCmd.NewCmdCompletion(f)) // Help topics cmd.AddCommand(NewHelpTopic("environment")) From 53a97c3414a4e78c66ddbc0db25afdd1f873781b Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 25 Sep 2020 16:11:11 +0200 Subject: [PATCH 3/6] Extract rootFlagErrrorFunc into root help --- pkg/cmd/root/help.go | 9 +++++++++ pkg/cmd/root/root.go | 15 ++++----------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/root/help.go b/pkg/cmd/root/help.go index 400be3834..ae57c1328 100644 --- a/pkg/cmd/root/help.go +++ b/pkg/cmd/root/help.go @@ -5,9 +5,11 @@ import ( "fmt" "strings" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/text" "github.com/cli/cli/utils" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) func rootUsageFunc(command *cobra.Command) error { @@ -33,6 +35,13 @@ func rootUsageFunc(command *cobra.Command) error { return nil } +func rootFlagErrrorFunc(cmd *cobra.Command, err error) error { + if err == pflag.ErrHelp { + return err + } + return &cmdutil.FlagError{Err: err} +} + var hasFailed bool // HasFailed signals that the main process should exit with non-zero status diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 8549ac3bc..749c3bc05 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -24,7 +24,6 @@ import ( creditsCmd "github.com/cli/cli/pkg/cmd/repo/credits" "github.com/cli/cli/pkg/cmdutil" "github.com/spf13/cobra" - "github.com/spf13/pflag" ) func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { @@ -73,13 +72,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.PersistentFlags().Bool("help", false, "Show help for command") cmd.SetHelpFunc(rootHelpFunc) cmd.SetUsageFunc(rootUsageFunc) - - cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { - if err == pflag.ErrHelp { - return err - } - return &cmdutil.FlagError{Err: err} - }) + cmd.SetFlagErrorFunc(rootFlagErrrorFunc) cmdutil.DisableAuthCheck(cmd) @@ -91,9 +84,6 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.AddCommand(gistCmd.NewCmdGist(f)) cmd.AddCommand(completionCmd.NewCmdCompletion(f)) - // Help topics - cmd.AddCommand(NewHelpTopic("environment")) - // the `api` command should not inherit any extra HTTP headers bareHTTPCmdFactory := *f bareHTTPCmdFactory.HttpClient = bareHTTPClient(f, version) @@ -109,6 +99,9 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.AddCommand(releaseCmd.NewCmdRelease(&repoResolvingCmdFactory)) cmd.AddCommand(repoCmd.NewCmdRepo(&repoResolvingCmdFactory)) + // Help topics + cmd.AddCommand(NewHelpTopic("environment")) + return cmd } From 1a1a6303cd3e1ce924bc14c2782fae0730c41a82 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 25 Sep 2020 16:21:55 +0200 Subject: [PATCH 4/6] Move disable auth check to bottom --- pkg/cmd/root/root.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 749c3bc05..b5b51970f 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -74,8 +74,6 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.SetUsageFunc(rootUsageFunc) cmd.SetFlagErrorFunc(rootFlagErrrorFunc) - cmdutil.DisableAuthCheck(cmd) - // Child commands cmd.AddCommand(aliasCmd.NewCmdAlias(f)) cmd.AddCommand(authCmd.NewCmdAuth(f)) @@ -102,6 +100,8 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { // Help topics cmd.AddCommand(NewHelpTopic("environment")) + cmdutil.DisableAuthCheck(cmd) + return cmd } From bda27a46ae5f1658e2d314b0c5fa79bb1ec58cb9 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 28 Sep 2020 11:57:31 +0200 Subject: [PATCH 5/6] Extract version command --- pkg/cmd/root/root.go | 38 +++------------- pkg/cmd/version/version.go | 45 +++++++++++++++++++ .../root_test.go => version/version_test.go} | 2 +- 3 files changed, 53 insertions(+), 32 deletions(-) create mode 100644 pkg/cmd/version/version.go rename pkg/cmd/{root/root_test.go => version/version_test.go} (98%) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index b5b51970f..16276fd22 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -1,10 +1,7 @@ package root import ( - "fmt" "net/http" - "regexp" - "strings" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" @@ -22,6 +19,7 @@ import ( releaseCmd "github.com/cli/cli/pkg/cmd/release" repoCmd "github.com/cli/cli/pkg/cmd/repo" creditsCmd "github.com/cli/cli/pkg/cmd/repo/credits" + versionCmd "github.com/cli/cli/pkg/cmd/version" "github.com/cli/cli/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -49,23 +47,6 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { }, } - version = strings.TrimPrefix(version, "v") - if buildDate == "" { - cmd.Version = version - } else { - cmd.Version = fmt.Sprintf("%s (%s)", version, buildDate) - } - versionOutput := fmt.Sprintf("gh version %s\n%s\n", cmd.Version, changelogURL(version)) - cmd.AddCommand(&cobra.Command{ - Use: "version", - Hidden: true, - Run: func(cmd *cobra.Command, args []string) { - fmt.Print(versionOutput) - }, - }) - cmd.SetVersionTemplate(versionOutput) - cmd.Flags().Bool("version", false, "Show gh version") - cmd.SetOut(f.IOStreams.Out) cmd.SetErr(f.IOStreams.ErrOut) @@ -74,7 +55,13 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.SetUsageFunc(rootUsageFunc) cmd.SetFlagErrorFunc(rootFlagErrrorFunc) + formattedVersion := versionCmd.Format(version, buildDate) + cmd.SetVersionTemplate(formattedVersion) + cmd.Version = formattedVersion + cmd.Flags().Bool("version", false, "Show gh version") + // Child commands + cmd.AddCommand(versionCmd.NewCmdVersion(f, version, buildDate)) cmd.AddCommand(aliasCmd.NewCmdAlias(f)) cmd.AddCommand(authCmd.NewCmdAuth(f)) cmd.AddCommand(configCmd.NewCmdConfig(f)) @@ -140,14 +127,3 @@ func resolvedBaseRepo(f *cmdutil.Factory) func() (ghrepo.Interface, error) { return baseRepo, nil } } - -func changelogURL(version string) string { - path := "https://github.com/cli/cli" - r := regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[\w.]+)?$`) - if !r.MatchString(version) { - return fmt.Sprintf("%s/releases/latest", path) - } - - url := fmt.Sprintf("%s/releases/tag/v%s", path, strings.TrimPrefix(version, "v")) - return url -} diff --git a/pkg/cmd/version/version.go b/pkg/cmd/version/version.go new file mode 100644 index 000000000..040a3132e --- /dev/null +++ b/pkg/cmd/version/version.go @@ -0,0 +1,45 @@ +package version + +import ( + "fmt" + "regexp" + "strings" + + "github.com/cli/cli/pkg/cmdutil" + "github.com/spf13/cobra" +) + +func NewCmdVersion(f *cmdutil.Factory, version, buildDate string) *cobra.Command { + cmd := &cobra.Command{ + Use: "version", + Hidden: true, + Run: func(cmd *cobra.Command, args []string) { + fmt.Fprint(f.IOStreams.Out, Format(version, buildDate)) + }, + } + + cmdutil.DisableAuthCheck(cmd) + + return cmd +} + +func Format(version, buildDate string) string { + version = strings.TrimPrefix(version, "v") + + if buildDate != "" { + version = fmt.Sprintf("%s (%s)", version, buildDate) + } + + return fmt.Sprintf("gh version %s\n%s\n", version, changelogURL(version)) +} + +func changelogURL(version string) string { + path := "https://github.com/cli/cli" + r := regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[\w.]+)?$`) + if !r.MatchString(version) { + return fmt.Sprintf("%s/releases/latest", path) + } + + url := fmt.Sprintf("%s/releases/tag/v%s", path, strings.TrimPrefix(version, "v")) + return url +} diff --git a/pkg/cmd/root/root_test.go b/pkg/cmd/version/version_test.go similarity index 98% rename from pkg/cmd/root/root_test.go rename to pkg/cmd/version/version_test.go index 714032ec3..9a1d49db3 100644 --- a/pkg/cmd/root/root_test.go +++ b/pkg/cmd/version/version_test.go @@ -1,4 +1,4 @@ -package root +package version import ( "testing" From 74d6eb96be080ae01a7e862129c88739cb9aa5e4 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Wed, 30 Sep 2020 12:52:45 +0200 Subject: [PATCH 6/6] Address PR comments --- pkg/cmd/completion/completion.go | 4 ++-- pkg/cmd/completion/completion_test.go | 6 +----- pkg/cmd/root/root.go | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/completion/completion.go b/pkg/cmd/completion/completion.go index 7c1ec46f0..3d3699b5d 100644 --- a/pkg/cmd/completion/completion.go +++ b/pkg/cmd/completion/completion.go @@ -6,11 +6,11 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" "github.com/spf13/cobra" ) -func NewCmdCompletion(f *cmdutil.Factory) *cobra.Command { - io := f.IOStreams +func NewCmdCompletion(io *iostreams.IOStreams) *cobra.Command { var shellType string cmd := &cobra.Command{ diff --git a/pkg/cmd/completion/completion_test.go b/pkg/cmd/completion/completion_test.go index d75c71398..3ce03f9b6 100644 --- a/pkg/cmd/completion/completion_test.go +++ b/pkg/cmd/completion/completion_test.go @@ -4,7 +4,6 @@ import ( "strings" "testing" - "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/google/shlex" "github.com/spf13/cobra" @@ -46,10 +45,7 @@ func TestNewCmdCompletion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { io, _, stdout, stderr := iostreams.Test() - f := &cmdutil.Factory{ - IOStreams: io, - } - completeCmd := NewCmdCompletion(f) + completeCmd := NewCmdCompletion(io) rootCmd := &cobra.Command{Use: "gh"} rootCmd.AddCommand(completeCmd) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 16276fd22..7a82f0cbf 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -67,7 +67,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { cmd.AddCommand(configCmd.NewCmdConfig(f)) cmd.AddCommand(creditsCmd.NewCmdCredits(f, nil)) cmd.AddCommand(gistCmd.NewCmdGist(f)) - cmd.AddCommand(completionCmd.NewCmdCompletion(f)) + cmd.AddCommand(completionCmd.NewCmdCompletion(f.IOStreams)) // the `api` command should not inherit any extra HTTP headers bareHTTPCmdFactory := *f