From 506bd4b2debe96596b2abfbb2d67248044c4a2ad Mon Sep 17 00:00:00 2001 From: bchadwic Date: Tue, 8 Jun 2021 15:36:11 -0700 Subject: [PATCH] working on refactoring the output, the tests and how opts is structured --- pkg/cmd/browse/browse.go | 99 +++++++++++++++-------------------- pkg/cmd/browse/browse_test.go | 30 +++++------ 2 files changed, 55 insertions(+), 74 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index d679abd4b..6d7b3df15 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -24,13 +24,13 @@ type BrowseOptions struct { BaseRepo func() (ghrepo.Interface, error) Browser browser - SelectorArg string - AdditionalArg string + SelectorArg string + FlagAmount int ProjectsFlag bool WikiFlag bool SettingsFlag bool - BranchFlag bool + Branch string } func NewCmdBrowse(f *cmdutil.Factory) *cobra.Command { @@ -75,91 +75,90 @@ func NewCmdBrowse(f *cmdutil.Factory) *cobra.Command { `), }, RunE: func(cmd *cobra.Command, args []string) error { - - if len(args) > 1 { - opts.AdditionalArg = args[1] + opts.FlagAmount = cmd.Flags().NFlag() + if opts.FlagAmount > 1 { + return &cmdutil.FlagError{Err: fmt.Errorf("accepts 1 flag, %d flag(s) were recieved", opts.FlagAmount)} } - + // TODO test whether you can set cobra command arg range to 1 with a branch accepting an arg1 if len(args) > 0 { opts.SelectorArg = args[0] } - return openInBrowser(cmd, opts) + + return runBrowse(opts) }, } cmdutil.EnableRepoOverride(cmd, f) cmd.Flags().BoolVarP(&opts.ProjectsFlag, "projects", "p", false, "Open repository projects") cmd.Flags().BoolVarP(&opts.WikiFlag, "wiki", "w", false, "Open repository wiki") cmd.Flags().BoolVarP(&opts.SettingsFlag, "settings", "s", false, "Open repository settings") - cmd.Flags().BoolVarP(&opts.BranchFlag, "branch", "b", false, "Select another branch by passing in the branch name") + cmd.Flags().StringVarP(&opts.Branch, "branch", "b", "", "Select another branch by passing in the branch name") return cmd } -func openInBrowser(cmd *cobra.Command, opts *BrowseOptions) error { +func runBrowse(opts *BrowseOptions) error { help := "Use 'gh browse --help' for more information about browse\n" + baseRepo, err := opts.BaseRepo() if err != nil { return fmt.Errorf("unable to determine base repository: %w\n%s", err, help) } httpClient, _ := opts.HttpClient() - apiClient := api.NewClientFromHTTP(httpClient) - branchName, _ := api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return fmt.Errorf("unable to create an http client: %w\n%s", err, help) + } - if getFlagAmount(cmd) > 1 { - return fmt.Errorf("accepts 1 flag, %d flag(s) were recieved\n%s", getFlagAmount(cmd), help) + apiClient := api.NewClientFromHTTP(httpClient) + branchName, err := api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return fmt.Errorf("unable to determine default branch for %s: %w", ghrepo.FullName(baseRepo), err) } repoUrl := ghrepo.GenerateRepoURL(baseRepo, "") - if !hasArg(opts) && hasFlag(cmd) { + hasArg := hasArg(opts) + hasFlag := hasFlag(opts) + + if hasArg && !hasFlag { + repoUrl = addArg(opts, repoUrl, branchName) + } else if !hasArg && hasFlag { err, repoUrl = addFlag(opts, repoUrl) if err != nil { return err } - } else if hasArg(opts) && !hasFlag(cmd) { - err, repoUrl = addArg(opts, repoUrl, branchName) + } else if hasArg && hasFlag { + err, repoUrl = addBranch(opts, repoUrl) if err != nil { return err } - } else if hasArg(opts) && hasFlag(cmd) { - err, repoUrl = addCombined(opts, repoUrl, branchName) - if err != nil { - return err - } - } - - if repoUrl == ghrepo.GenerateRepoURL(baseRepo, "") { + } else { fmt.Fprintf(opts.IO.Out, "now opening %s in browser . . .\n", repoUrl) } + opts.Browser.Browse(repoUrl) return nil } -func addCombined(opts *BrowseOptions, url string, branchName string) (error, string) { +func addBranch(opts *BrowseOptions, url string) (error, string) { help := "Use 'gh browse --help' for more information about browse\n" - if !opts.BranchFlag { + if opts.Branch == "" { return fmt.Errorf("invalid use of flag and argument\n%s", help), "" } - if opts.AdditionalArg == "" { - url += "/tree/" + opts.SelectorArg + if opts.SelectorArg == "" { + url += "/tree/" + opts.Branch fmt.Fprintf(opts.IO.Out, "now opening %s in browser . . .\n", url) return nil, url } arr := parseFileArg(opts) if len(arr) > 1 { - url += "/tree/" + opts.SelectorArg - fmt.Fprintf(opts.IO.Out, "now opening %s in browser . . .\n", url) - return nil, url + return nil, url + "/tree/" + opts.Branch + "/" + arr[0] + "#L" + arr[1] } - - url += "/tree/" + opts.SelectorArg - fmt.Fprintf(opts.IO.Out, "now opening %s in browser . . .\n", url) - return nil, url + return nil, url + "/tree/" + opts.Branch + "/" + arr[0] } func addFlag(opts *BrowseOptions, url string) (error, string) { @@ -174,27 +173,21 @@ func addFlag(opts *BrowseOptions, url string) (error, string) { return nil, url } -func addArg(opts *BrowseOptions, url string, branchName string) (error, string) { - help := "Use 'gh browse --help' for more information about browse\n" - - if opts.AdditionalArg != "" { - return fmt.Errorf("accepts 1 arg, 2 arg(s) were recieved\n%s", help), "" - } - +func addArg(opts *BrowseOptions, url string, branchName string) string { if isNumber(opts.SelectorArg) { url += "/issues/" + opts.SelectorArg fmt.Fprintf(opts.IO.Out, "now opening issue/pr in browser . . .\n") - return nil, url + return url } arr := parseFileArg(opts) if len(arr) > 1 { fmt.Fprintf(opts.IO.Out, "now opening %s in browser . . .\n", url) - return nil, url + "/tree/" + branchName + "/" + arr[0] + "#L" + arr[1] + return url + "/tree/" + branchName + "/" + arr[0] + "#L" + arr[1] } fmt.Fprintf(opts.IO.Out, "now opening %s in browser . . .\n", url) - return nil, url + "/tree/" + branchName + "/" + arr[0] + return url + "/tree/" + branchName + "/" + arr[0] } func parseFileArg(opts *BrowseOptions) []string { @@ -202,23 +195,15 @@ func parseFileArg(opts *BrowseOptions) []string { return arr } -func hasFlag(cmd *cobra.Command) bool { - return getFlagAmount(cmd) > 0 +func hasFlag(opts *BrowseOptions) bool { + return opts.FlagAmount > 0 } func hasArg(opts *BrowseOptions) bool { return opts.SelectorArg != "" } -func getFlagAmount(cmd *cobra.Command) int { - return cmd.Flags().NFlag() -} - func isNumber(arg string) bool { _, err := strconv.Atoi(arg) - if err != nil { - return false - } else { - return true - } + return err == nil } diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index d1c9442c3..0912884ca 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -6,7 +6,6 @@ import ( "net/http" "testing" - "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/cmdutil" @@ -41,9 +40,6 @@ func runCommand(rt http.RoundTripper, t testCase) (*test.CmdOut, error) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: rt}, nil }, - Config: func() (config.Config, error) { - return config.NewBlankConfig(), nil - }, BaseRepo: func() (ghrepo.Interface, error) { return t.args.repo, nil }, @@ -78,7 +74,7 @@ func TestNewCmdBrowse(t *testing.T) { }, errorExpected: true, stdoutExpected: "", - stderrExpected: "accepts 1 flag, 2 flag(s) were recieved\nUse 'gh browse --help' for more information about browse\n", + stderrExpected: "accepts 1 flag, 2 flag(s) were recieved", urlExpected: "", }, { @@ -114,17 +110,17 @@ func TestNewCmdBrowse(t *testing.T) { stderrExpected: "", urlExpected: "https://github.com/thanh/cli/wiki", }, - { - name: "branch flag", - args: args{ - repo: ghrepo.New("ken", "cli"), - cli: "--branch", - }, - errorExpected: false, - stdoutExpected: "", - stderrExpected: "Aa", - urlExpected: "", - }, + // { + // name: "branch flag", + // args: args{ + // repo: ghrepo.New("ken", "cli"), + // cli: "--branch", + // }, + // errorExpected: false, + // stdoutExpected: "", + // stderrExpected: "Aa", + // urlExpected: "", + // }, } for _, tt := range tests { @@ -142,7 +138,7 @@ func TestNewCmdBrowse(t *testing.T) { } else { assert.Equal(t, err, nil) } - assert.Equal(t, output.OutBuf.String(), tt.stdoutExpected) + assert.Equal(t, tt.stdoutExpected, output.OutBuf.String()) assert.Equal(t, tt.urlExpected, output.BrowsedURL) }) }