diff --git a/pkg/cmd/issue/argparsetest/argparsetest.go b/pkg/cmd/issue/argparsetest/argparsetest.go new file mode 100644 index 000000000..5ae1ada8d --- /dev/null +++ b/pkg/cmd/issue/argparsetest/argparsetest.go @@ -0,0 +1,137 @@ +package argparsetest + +import ( + "bytes" + "reflect" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/google/shlex" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newCmdFunc represents the typical function signature we use for creating commands e.g. `NewCmdView`. +// +// It is generic over `T` as each command construction has their own Options type e.g. `ViewOptions` +type newCmdFunc[T any] func(f *cmdutil.Factory, runF func(*T) error) *cobra.Command + +// TestArgParsing is a test helper that verifies that issue commands correctly parse the `{issue number | url}` +// positional arg into an issue number and base repo. +// +// Looking through the existing tests, I noticed that the coverage was pretty smattered. +// Since nearly all issue commands only accept a single positional argument, we are able to reuse this test helper. +// Commands with no further flags or args can use this solely. +// Commands with extra flags use this and further table tests. +// Commands with extra required positional arguments (like `transfer`) cannot use this. They duplicate these cases inline. +func TestArgParsing[T any](t *testing.T, fn newCmdFunc[T]) { + tests := []struct { + name string + input string + expectedissueNumber int + expectedBaseRepo ghrepo.Interface + expectErr bool + }{ + { + name: "no argument", + input: "", + expectErr: true, + }, + { + name: "issue number argument", + input: "23 --repo owner/repo", + expectedissueNumber: 23, + expectedBaseRepo: ghrepo.New("owner", "repo"), + }, + { + name: "argument is hash prefixed number", + // Escaping is required here to avoid what I think is shellex treating it as a comment. + input: "\\#23 --repo owner/repo", + expectedissueNumber: 23, + expectedBaseRepo: ghrepo.New("owner", "repo"), + }, + { + name: "argument is a URL", + input: "https://github.com/cli/cli/issues/23", + expectedissueNumber: 23, + expectedBaseRepo: ghrepo.New("cli", "cli"), + }, + { + name: "argument cannot be parsed to an issue", + input: "unparseable", + expectErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &cmdutil.Factory{} + + argv, err := shlex.Split(tt.input) + assert.NoError(t, err) + + var gotOpts T + cmd := fn(f, func(opts *T) error { + gotOpts = *opts + return nil + }) + + cmdutil.EnableRepoOverride(cmd, f) + + // TODO: remember why we do this + cmd.Flags().BoolP("help", "x", false, "") + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + + if tt.expectErr { + require.Error(t, err) + return + } else { + require.NoError(t, err) + } + + actualIssueNumber := issueNumberFromOpts(t, gotOpts) + assert.Equal(t, tt.expectedissueNumber, actualIssueNumber) + + actualBaseRepo := baseRepoFromOpts(t, gotOpts) + assert.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, actualBaseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, actualBaseRepo, + ) + }) + } +} + +func issueNumberFromOpts(t *testing.T, v any) int { + rv := reflect.ValueOf(v) + field := rv.FieldByName("IssueNumber") + if !field.IsValid() || field.Kind() != reflect.Int { + t.Fatalf("Type %T does not have IssueNumber int field", v) + } + return int(field.Int()) +} + +func baseRepoFromOpts(t *testing.T, v any) ghrepo.Interface { + rv := reflect.ValueOf(v) + field := rv.FieldByName("BaseRepo") + // check whether the field is valid and of type func() (ghrepo.Interface, error) + if !field.IsValid() || field.Kind() != reflect.Func { + t.Fatalf("Type %T does not have BaseRepo func field", v) + } + // call the function and check the return value + results := field.Call([]reflect.Value{}) + if len(results) != 2 { + t.Fatalf("%T.BaseRepo() does not return two values", v) + } + if !results[1].IsNil() { + t.Fatalf("%T.BaseRepo() returned an error: %v", v, results[1].Interface()) + } + return results[0].Interface().(ghrepo.Interface) +} diff --git a/pkg/cmd/issue/close/close.go b/pkg/cmd/issue/close/close.go index 9197abff6..21fe45dd6 100644 --- a/pkg/cmd/issue/close/close.go +++ b/pkg/cmd/issue/close/close.go @@ -21,7 +21,7 @@ type CloseOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - SelectorArg string + IssueNumber int Comment string Reason string @@ -39,13 +39,23 @@ func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Comm Short: "Close issue", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo - - if len(args) > 0 { - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err } + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + if runF != nil { return runF(opts) } @@ -67,7 +77,12 @@ func closeRun(opts *CloseOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "state"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title", "state"}) if err != nil { return err } diff --git a/pkg/cmd/issue/close/close_test.go b/pkg/cmd/issue/close/close_test.go index 4d50e56b2..04c39cd8d 100644 --- a/pkg/cmd/issue/close/close_test.go +++ b/pkg/cmd/issue/close/close_test.go @@ -7,46 +7,32 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdClose(t *testing.T) { + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdClose) + tests := []struct { - name string - input string - output CloseOptions - wantErr bool - errMsg string + name string + input string + output CloseOptions + expectedBaseRepo ghrepo.Interface + wantErr bool + errMsg string }{ - { - name: "no argument", - input: "", - wantErr: true, - errMsg: "accepts 1 arg(s), received 0", - }, - { - name: "issue number", - input: "123", - output: CloseOptions{ - SelectorArg: "123", - }, - }, - { - name: "issue url", - input: "https://github.com/cli/cli/3", - output: CloseOptions{ - SelectorArg: "https://github.com/cli/cli/3", - }, - }, { name: "comment", input: "123 --comment 'closing comment'", output: CloseOptions{ - SelectorArg: "123", + IssueNumber: 123, Comment: "closing comment", }, }, @@ -54,7 +40,7 @@ func TestNewCmdClose(t *testing.T) { name: "reason", input: "123 --reason 'not planned'", output: CloseOptions{ - SelectorArg: "123", + IssueNumber: 123, Reason: "not planned", }, }, @@ -79,15 +65,24 @@ func TestNewCmdClose(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantErr { - assert.Error(t, err) + require.Error(t, err) assert.Equal(t, tt.errMsg, err.Error()) return } - assert.NoError(t, err) - assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg) + require.NoError(t, err) + assert.Equal(t, tt.output.IssueNumber, gotOpts.IssueNumber) assert.Equal(t, tt.output.Comment, gotOpts.Comment) assert.Equal(t, tt.output.Reason, gotOpts.Reason) + if tt.expectedBaseRepo != nil { + baseRepo, err := gotOpts.BaseRepo() + require.NoError(t, err) + require.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, baseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo, + ) + } }) } } @@ -104,7 +99,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue by number", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, }, httpStubs: func(reg *httpmock.Registry) { reg.Register( @@ -128,7 +123,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue with comment", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, Comment: "closing comment", }, httpStubs: func(reg *httpmock.Registry) { @@ -164,7 +159,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue with reason", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, Reason: "not planned", Detector: &fd.EnabledDetectorMock{}, }, @@ -192,7 +187,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue with reason when reason is not supported", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, Reason: "not planned", Detector: &fd.DisabledDetectorMock{}, }, @@ -219,7 +214,7 @@ func TestCloseRun(t *testing.T) { { name: "issue already closed", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, }, httpStubs: func(reg *httpmock.Registry) { reg.Register( @@ -236,7 +231,7 @@ func TestCloseRun(t *testing.T) { { name: "issues disabled", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, }, httpStubs: func(reg *httpmock.Registry) { reg.Register( diff --git a/pkg/cmd/issue/comment/comment.go b/pkg/cmd/issue/comment/comment.go index 090b0748c..706ff791e 100644 --- a/pkg/cmd/issue/comment/comment.go +++ b/pkg/cmd/issue/comment/comment.go @@ -3,6 +3,7 @@ package comment import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/issue/shared" issueShared "github.com/cli/cli/v2/pkg/cmd/issue/shared" prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -37,15 +38,41 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*prShared.CommentableOptions) e Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { opts.RetrieveCommentable = func() (prShared.Commentable, ghrepo.Interface, error) { + // TODO wm: more testing + issueNumber, parsedBaseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return nil, nil, err + } + + // If the args provided the base repo then use that directly. + var baseRepo ghrepo.Interface + + if parsedBaseRepo, present := parsedBaseRepo.Value(); present { + baseRepo = parsedBaseRepo + } else { + // support `-R, --repo` override + baseRepo, err = f.BaseRepo() + if err != nil { + return nil, nil, err + } + } + httpClient, err := f.HttpClient() if err != nil { return nil, nil, err } + fields := []string{"id", "url"} if opts.EditLast { fields = append(fields, "comments") } - return issueShared.IssueFromArgWithFields(httpClient, f.BaseRepo, args[0], fields) + + issue, err := issueShared.FindIssueOrPR(httpClient, baseRepo, issueNumber, fields) + if err != nil { + return nil, nil, err + } + + return issue, baseRepo, nil } return prShared.CommentablePreRun(cmd, opts) }, diff --git a/pkg/cmd/issue/delete/delete.go b/pkg/cmd/issue/delete/delete.go index fb41f288e..269ef7081 100644 --- a/pkg/cmd/issue/delete/delete.go +++ b/pkg/cmd/issue/delete/delete.go @@ -21,7 +21,7 @@ type DeleteOptions struct { BaseRepo func() (ghrepo.Interface, error) Prompter iprompter - SelectorArg string + IssueNumber int Confirmed bool } @@ -42,13 +42,23 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co Short: "Delete issue", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo - - if len(args) > 0 { - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err } + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + if runF != nil { return runF(opts) } @@ -71,7 +81,12 @@ func deleteRun(opts *DeleteOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title"}) if err != nil { return err } diff --git a/pkg/cmd/issue/delete/delete_test.go b/pkg/cmd/issue/delete/delete_test.go index bd83c826f..64522b1d3 100644 --- a/pkg/cmd/issue/delete/delete_test.go +++ b/pkg/cmd/issue/delete/delete_test.go @@ -12,6 +12,7 @@ import ( "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -20,6 +21,10 @@ import ( "github.com/stretchr/testify/assert" ) +func TestNewCmdDelete(t *testing.T) { + argparsetest.TestArgParsing(t, NewCmdDelete) +} + func runCommand(rt http.RoundTripper, pm *prompter.MockPrompter, isTTY bool, cli string) (*test.CmdOut, error) { ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(isTTY) diff --git a/pkg/cmd/issue/develop/develop.go b/pkg/cmd/issue/develop/develop.go index 1536800f0..19c9b5fa9 100644 --- a/pkg/cmd/issue/develop/develop.go +++ b/pkg/cmd/issue/develop/develop.go @@ -24,12 +24,12 @@ type DevelopOptions struct { BaseRepo func() (ghrepo.Interface, error) Remotes func() (context.Remotes, error) - IssueSelector string - Name string - BranchRepo string - BaseBranch string - Checkout bool - List bool + IssueNumber int + Name string + BranchRepo string + BaseBranch string + Checkout bool + List bool } func NewCmdDevelop(f *cmdutil.Factory, runF func(*DevelopOptions) error) *cobra.Command { @@ -89,9 +89,23 @@ func NewCmdDevelop(f *cmdutil.Factory, runF func(*DevelopOptions) error) *cobra. return nil }, RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo - opts.IssueSelector = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err + } + + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + if err := cmdutil.MutuallyExclusive("specify only one of `--list` or `--branch-repo`", opts.List, opts.BranchRepo != ""); err != nil { return err } @@ -131,8 +145,13 @@ func developRun(opts *DevelopOptions) error { return err } + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + opts.IO.StartProgressIndicator() - issue, issueRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.IssueSelector, []string{"id", "number"}) + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number"}) opts.IO.StopProgressIndicator() if err != nil { return err @@ -141,16 +160,16 @@ func developRun(opts *DevelopOptions) error { apiClient := api.NewClientFromHTTP(httpClient) opts.IO.StartProgressIndicator() - err = api.CheckLinkedBranchFeature(apiClient, issueRepo.RepoHost()) + err = api.CheckLinkedBranchFeature(apiClient, baseRepo.RepoHost()) opts.IO.StopProgressIndicator() if err != nil { return err } if opts.List { - return developRunList(opts, apiClient, issueRepo, issue) + return developRunList(opts, apiClient, baseRepo, issue) } - return developRunCreate(opts, apiClient, issueRepo, issue) + return developRunCreate(opts, apiClient, baseRepo, issue) } func developRunCreate(opts *DevelopOptions, apiClient *api.Client, issueRepo ghrepo.Interface, issue *api.Issue) error { diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index 831f03fc3..2485c8cc4 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -11,89 +11,74 @@ import ( "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/run" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdDevelop(t *testing.T) { + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdDevelop) + tests := []struct { - name string - input string - output DevelopOptions - wantStdout string - wantStderr string - wantErr bool - errMsg string + name string + input string + output DevelopOptions + expectedBaseRepo ghrepo.Interface + wantStdout string + wantStderr string + wantErr bool + errMsg string }{ - { - name: "no argument", - input: "", - output: DevelopOptions{}, - wantErr: true, - errMsg: "issue number or url is required", - }, - { - name: "issue number", - input: "1", - output: DevelopOptions{ - IssueSelector: "1", - }, - }, - { - name: "issue url", - input: "https://github.com/cli/cli/issues/1", - output: DevelopOptions{ - IssueSelector: "https://github.com/cli/cli/issues/1", - }, - }, { name: "branch-repo flag", input: "1 --branch-repo owner/repo", output: DevelopOptions{ - IssueSelector: "1", - BranchRepo: "owner/repo", + IssueNumber: 1, + BranchRepo: "owner/repo", }, }, { name: "base flag", input: "1 --base feature", output: DevelopOptions{ - IssueSelector: "1", - BaseBranch: "feature", + IssueNumber: 1, + BaseBranch: "feature", }, }, { name: "checkout flag", input: "1 --checkout", output: DevelopOptions{ - IssueSelector: "1", - Checkout: true, + IssueNumber: 1, + Checkout: true, }, }, { name: "list flag", input: "1 --list", output: DevelopOptions{ - IssueSelector: "1", - List: true, + IssueNumber: 1, + List: true, }, }, { name: "name flag", input: "1 --name feature", output: DevelopOptions{ - IssueSelector: "1", - Name: "feature", + IssueNumber: 1, + Name: "feature", }, }, { name: "issue-repo flag", input: "1 --issue-repo cli/cli", output: DevelopOptions{ - IssueSelector: "1", + IssueNumber: 1, }, wantStdout: "Flag --issue-repo has been deprecated, use `--repo` instead\n", }, @@ -143,18 +128,27 @@ func TestNewCmdDevelop(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantErr { - assert.EqualError(t, err, tt.errMsg) + require.EqualError(t, err, tt.errMsg) return } - assert.NoError(t, err) - assert.Equal(t, tt.output.IssueSelector, gotOpts.IssueSelector) + require.NoError(t, err) + assert.Equal(t, tt.output.IssueNumber, gotOpts.IssueNumber) assert.Equal(t, tt.output.Name, gotOpts.Name) assert.Equal(t, tt.output.BaseBranch, gotOpts.BaseBranch) assert.Equal(t, tt.output.Checkout, gotOpts.Checkout) assert.Equal(t, tt.output.List, gotOpts.List) assert.Equal(t, tt.wantStdout, stdOut.String()) assert.Equal(t, tt.wantStderr, stdErr.String()) + if tt.expectedBaseRepo != nil { + baseRepo, err := gotOpts.BaseRepo() + require.NoError(t, err) + require.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, baseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo, + ) + } }) } } @@ -178,8 +172,8 @@ func TestDevelopRun(t *testing.T) { { name: "returns an error when the feature is not supported by the API", opts: &DevelopOptions{ - IssueSelector: "42", - List: true, + IssueNumber: 42, + List: true, }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( @@ -196,8 +190,8 @@ func TestDevelopRun(t *testing.T) { { name: "list branches for an issue", opts: &DevelopOptions{ - IssueSelector: "42", - List: true, + IssueNumber: 42, + List: true, }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( @@ -223,8 +217,8 @@ func TestDevelopRun(t *testing.T) { { name: "list branches for an issue in tty", opts: &DevelopOptions{ - IssueSelector: "42", - List: true, + IssueNumber: 42, + List: true, }, tty: true, httpStubs: func(reg *httpmock.Registry, t *testing.T) { @@ -255,37 +249,10 @@ func TestDevelopRun(t *testing.T) { bar https://github.com/OWNER/OTHER-REPO/tree/bar `), }, - { - name: "list branches for an issue providing an issue url", - opts: &DevelopOptions{ - IssueSelector: "https://github.com/cli/cli/issues/42", - List: true, - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{"hasIssuesEnabled":true,"issue":{"id":"SOMEID","number":42}}}}`), - ) - reg.Register( - httpmock.GraphQL(`query LinkedBranchFeature\b`), - httpmock.StringResponse(featureEnabledPayload), - ) - reg.Register( - httpmock.GraphQL(`query ListLinkedBranches\b`), - httpmock.GraphQLQuery(` - {"data":{"repository":{"issue":{"linkedBranches":{"nodes":[{"ref":{"name":"foo","repository":{"url":"https://github.com/OWNER/REPO"}}},{"ref":{"name":"bar","repository":{"url":"https://github.com/OWNER/OTHER-REPO"}}}]}}}}} - `, func(query string, inputs map[string]interface{}) { - assert.Equal(t, float64(42), inputs["number"]) - assert.Equal(t, "cli", inputs["owner"]) - assert.Equal(t, "cli", inputs["name"]) - })) - }, - expectedOut: "foo\thttps://github.com/OWNER/REPO/tree/foo\nbar\thttps://github.com/OWNER/OTHER-REPO/tree/bar\n", - }, { name: "develop new branch", opts: &DevelopOptions{ - IssueSelector: "123", + IssueNumber: 123, }, remotes: map[string]string{ "origin": "OWNER/REPO", @@ -321,8 +288,8 @@ func TestDevelopRun(t *testing.T) { { name: "develop new branch in different repo than issue", opts: &DevelopOptions{ - IssueSelector: "123", - BranchRepo: "OWNER2/REPO", + IssueNumber: 123, + BranchRepo: "OWNER2/REPO", }, remotes: map[string]string{ "origin": "OWNER2/REPO", @@ -367,9 +334,9 @@ func TestDevelopRun(t *testing.T) { { name: "develop new branch with name and base specified", opts: &DevelopOptions{ - Name: "my-branch", - BaseBranch: "main", - IssueSelector: "123", + Name: "my-branch", + BaseBranch: "main", + IssueNumber: 123, }, remotes: map[string]string{ "origin": "OWNER/REPO", @@ -406,7 +373,10 @@ func TestDevelopRun(t *testing.T) { { name: "develop new branch outside of local git repo", opts: &DevelopOptions{ - IssueSelector: "https://github.com/cli/cli/issues/123", + IssueNumber: 123, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("cli", "cli"), nil + }, }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( @@ -436,9 +406,9 @@ func TestDevelopRun(t *testing.T) { { name: "develop new branch with checkout when local branch exists", opts: &DevelopOptions{ - Name: "my-branch", - IssueSelector: "123", - Checkout: true, + Name: "my-branch", + IssueNumber: 123, + Checkout: true, }, remotes: map[string]string{ "origin": "OWNER/REPO", @@ -478,9 +448,9 @@ func TestDevelopRun(t *testing.T) { { name: "develop new branch with checkout when local branch does not exist", opts: &DevelopOptions{ - Name: "my-branch", - IssueSelector: "123", - Checkout: true, + Name: "my-branch", + IssueNumber: 123, + Checkout: true, }, remotes: map[string]string{ "origin": "OWNER/REPO", @@ -519,8 +489,8 @@ func TestDevelopRun(t *testing.T) { { name: "develop with base branch which does not exist", opts: &DevelopOptions{ - IssueSelector: "123", - BaseBranch: "does-not-exist-branch", + IssueNumber: 123, + BaseBranch: "does-not-exist-branch", }, remotes: map[string]string{ "origin": "OWNER/REPO", @@ -561,8 +531,10 @@ func TestDevelopRun(t *testing.T) { ios.SetStderrTTY(tt.tty) opts.IO = ios - opts.BaseRepo = func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil + if opts.BaseRepo == nil { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + } } opts.Remotes = func() (context.Remotes, error) { diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 18067319f..accea8add 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -28,7 +28,7 @@ type EditOptions struct { EditFieldsSurvey func(prShared.EditPrompter, *prShared.Editable, string) error FetchOptions func(*api.Client, ghrepo.Interface, *prShared.Editable) error - SelectorArgs []string + IssueNumbers []int Interactive bool prShared.Editable @@ -69,10 +69,22 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman `), Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + issueNumbers, baseRepo, err := shared.ParseIssuesFromArgs(args) + if err != nil { + return err + } - opts.SelectorArgs = args + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumbers = issueNumbers flags := cmd.Flags() @@ -134,7 +146,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman return cmdutil.FlagErrorf("field to edit flag required when not running interactively") } - if opts.Interactive && len(opts.SelectorArgs) > 1 { + if opts.Interactive && len(opts.IssueNumbers) > 1 { return cmdutil.FlagErrorf("multiple issues cannot be edited interactively") } @@ -167,6 +179,11 @@ func editRun(opts *EditOptions) error { return err } + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + // Prompt the user which fields they'd like to edit. editable := opts.Editable if opts.Interactive { @@ -192,7 +209,7 @@ func editRun(opts *EditOptions) error { } // Get all specified issues and make sure they are within the same repo. - issues, repo, err := shared.IssuesFromArgsWithFields(httpClient, opts.BaseRepo, opts.SelectorArgs, lookupFields) + issues, err := shared.FindIssuesOrPRs(httpClient, baseRepo, opts.IssueNumbers, lookupFields) if err != nil { return err } @@ -200,7 +217,7 @@ func editRun(opts *EditOptions) error { // Fetch editable shared fields once for all issues. apiClient := api.NewClientFromHTTP(httpClient) opts.IO.StartProgressIndicatorWithLabel("Fetching repository information") - err = opts.FetchOptions(apiClient, repo, &editable) + err = opts.FetchOptions(apiClient, baseRepo, &editable) opts.IO.StopProgressIndicator() if err != nil { return err @@ -250,7 +267,7 @@ func editRun(opts *EditOptions) error { go func(issue *api.Issue) { defer g.Done() - err := prShared.UpdateIssue(httpClient, repo, issue.ID, issue.IsPullRequest(), editable) + err := prShared.UpdateIssue(httpClient, baseRepo, issue.ID, issue.IsPullRequest(), editable) if err != nil { failedIssueChan <- fmt.Sprintf("failed to update %s: %s", issue.URL, err) return diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 40fe6491c..a9d43c3ec 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -26,11 +26,12 @@ func TestNewCmdEdit(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - input string - stdin string - output EditOptions - wantsErr bool + name string + input string + stdin string + output EditOptions + expectedBaseRepo ghrepo.Interface + wantsErr bool }{ { name: "no argument", @@ -42,7 +43,7 @@ func TestNewCmdEdit(t *testing.T) { name: "issue number argument", input: "23", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Interactive: true, }, wantsErr: false, @@ -51,7 +52,7 @@ func TestNewCmdEdit(t *testing.T) { name: "title flag", input: "23 --title test", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Title: prShared.EditableString{ Value: "test", @@ -65,7 +66,7 @@ func TestNewCmdEdit(t *testing.T) { name: "body flag", input: "23 --body test", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Body: prShared.EditableString{ Value: "test", @@ -80,7 +81,7 @@ func TestNewCmdEdit(t *testing.T) { input: "23 --body-file -", stdin: "this is on standard input", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Body: prShared.EditableString{ Value: "this is on standard input", @@ -94,7 +95,7 @@ func TestNewCmdEdit(t *testing.T) { name: "body from file", input: fmt.Sprintf("23 --body-file '%s'", tmpFile), output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Body: prShared.EditableString{ Value: "a body from file", @@ -113,7 +114,7 @@ func TestNewCmdEdit(t *testing.T) { name: "add-assignee flag", input: "23 --add-assignee monalisa,hubot", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Assignees: prShared.EditableSlice{ Add: []string{"monalisa", "hubot"}, @@ -127,7 +128,7 @@ func TestNewCmdEdit(t *testing.T) { name: "remove-assignee flag", input: "23 --remove-assignee monalisa,hubot", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Assignees: prShared.EditableSlice{ Remove: []string{"monalisa", "hubot"}, @@ -141,7 +142,7 @@ func TestNewCmdEdit(t *testing.T) { name: "add-label flag", input: "23 --add-label feature,TODO,bug", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Labels: prShared.EditableSlice{ Add: []string{"feature", "TODO", "bug"}, @@ -155,7 +156,7 @@ func TestNewCmdEdit(t *testing.T) { name: "remove-label flag", input: "23 --remove-label feature,TODO,bug", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Labels: prShared.EditableSlice{ Remove: []string{"feature", "TODO", "bug"}, @@ -169,7 +170,7 @@ func TestNewCmdEdit(t *testing.T) { name: "add-project flag", input: "23 --add-project Cleanup,Roadmap", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Projects: prShared.EditableProjects{ EditableSlice: prShared.EditableSlice{ @@ -185,7 +186,7 @@ func TestNewCmdEdit(t *testing.T) { name: "remove-project flag", input: "23 --remove-project Cleanup,Roadmap", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Projects: prShared.EditableProjects{ EditableSlice: prShared.EditableSlice{ @@ -201,7 +202,7 @@ func TestNewCmdEdit(t *testing.T) { name: "milestone flag", input: "23 --milestone GA", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Milestone: prShared.EditableString{ Value: "GA", @@ -215,7 +216,7 @@ func TestNewCmdEdit(t *testing.T) { name: "remove-milestone flag", input: "23 --remove-milestone", output: EditOptions{ - SelectorArgs: []string{"23"}, + IssueNumbers: []int{23}, Editable: prShared.Editable{ Milestone: prShared.EditableString{ Value: "", @@ -234,7 +235,7 @@ func TestNewCmdEdit(t *testing.T) { name: "add label to multiple issues", input: "23 34 --add-label bug", output: EditOptions{ - SelectorArgs: []string{"23", "34"}, + IssueNumbers: []int{23, 34}, Editable: prShared.Editable{ Labels: prShared.EditableSlice{ Add: []string{"bug"}, @@ -244,6 +245,31 @@ func TestNewCmdEdit(t *testing.T) { }, wantsErr: false, }, + { + name: "argument is hash prefixed number", + // Escaping is required here to avoid what I think is shellex treating it as a comment. + input: "\\#23", + output: EditOptions{ + IssueNumbers: []int{23}, + Interactive: true, + }, + wantsErr: false, + }, + { + name: "argument is a URL", + input: "https://github.com/cli/cli/issues/23", + output: EditOptions{ + IssueNumbers: []int{23}, + Interactive: true, + }, + expectedBaseRepo: ghrepo.New("cli", "cli"), + wantsErr: false, + }, + { + name: "URL arguments parse as different repos", + input: "https://github.com/cli/cli/issues/23 https://github.com/cli/go-gh/issues/23", + wantsErr: true, + }, { name: "interactive multiple issues", input: "23 34", @@ -282,14 +308,23 @@ func TestNewCmdEdit(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantsErr { - assert.Error(t, err) + require.Error(t, err) return } - assert.NoError(t, err) - assert.Equal(t, tt.output.SelectorArgs, gotOpts.SelectorArgs) + require.NoError(t, err) + assert.Equal(t, tt.output.IssueNumbers, gotOpts.IssueNumbers) assert.Equal(t, tt.output.Interactive, gotOpts.Interactive) assert.Equal(t, tt.output.Editable, gotOpts.Editable) + if tt.expectedBaseRepo != nil { + baseRepo, err := gotOpts.BaseRepo() + require.NoError(t, err) + require.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, baseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo, + ) + } }) } } @@ -306,7 +341,7 @@ func Test_editRun(t *testing.T) { { name: "non-interactive", input: &EditOptions{ - SelectorArgs: []string{"123"}, + IssueNumbers: []int{123}, Interactive: false, Editable: prShared.Editable{ Title: prShared.EditableString{ @@ -359,7 +394,7 @@ func Test_editRun(t *testing.T) { { name: "non-interactive multiple issues", input: &EditOptions{ - SelectorArgs: []string{"456", "123"}, + IssueNumbers: []int{456, 123}, Interactive: false, Editable: prShared.Editable{ Assignees: prShared.EditableSlice{ @@ -409,7 +444,7 @@ func Test_editRun(t *testing.T) { { name: "non-interactive multiple issues with fetch failures", input: &EditOptions{ - SelectorArgs: []string{"123", "9999"}, + IssueNumbers: []int{123, 9999}, Interactive: false, Editable: prShared.Editable{ Assignees: prShared.EditableSlice{ @@ -454,7 +489,7 @@ func Test_editRun(t *testing.T) { { name: "non-interactive multiple issues with update failures", input: &EditOptions{ - SelectorArgs: []string{"123", "456"}, + IssueNumbers: []int{123, 456}, Interactive: false, Editable: prShared.Editable{ Assignees: prShared.EditableSlice{ @@ -524,7 +559,7 @@ func Test_editRun(t *testing.T) { { name: "interactive", input: &EditOptions{ - SelectorArgs: []string{"123"}, + IssueNumbers: []int{123}, Interactive: true, FieldsToEditSurvey: func(p prShared.EditPrompter, eo *prShared.Editable) error { eo.Title.Edited = true diff --git a/pkg/cmd/issue/lock/lock.go b/pkg/cmd/issue/lock/lock.go index 4e0dac058..2f332d21d 100644 --- a/pkg/cmd/issue/lock/lock.go +++ b/pkg/cmd/issue/lock/lock.go @@ -19,6 +19,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/issue/shared" issueShared "github.com/cli/cli/v2/pkg/cmd/issue/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -99,20 +100,33 @@ type LockOptions struct { ParentCmd string Reason string - SelectorArg string + IssueNumber int Interactive bool } -func (opts *LockOptions) setCommonOptions(f *cmdutil.Factory, args []string) { +func (opts *LockOptions) setCommonOptions(f *cmdutil.Factory, args []string) error { opts.IO = f.IOStreams opts.HttpClient = f.HttpClient opts.Config = f.Config - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err + } - opts.SelectorArg = args[0] + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + opts.IssueNumber = issueNumber + + return nil } func NewCmdLock(f *cmdutil.Factory, parentName string, runF func(string, *LockOptions) error) *cobra.Command { @@ -129,7 +143,9 @@ func NewCmdLock(f *cmdutil.Factory, parentName string, runF func(string, *LockOp Short: short, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - opts.setCommonOptions(f, args) + if err := opts.setCommonOptions(f, args); err != nil { + return err + } reasonProvided := cmd.Flags().Changed("reason") if reasonProvided { @@ -172,7 +188,9 @@ func NewCmdUnlock(f *cmdutil.Factory, parentName string, runF func(string, *Lock Short: short, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - opts.setCommonOptions(f, args) + if err := opts.setCommonOptions(f, args); err != nil { + return err + } if runF != nil { return runF(Unlock, opts) @@ -214,13 +232,18 @@ func lockRun(state string, opts *LockOptions) error { return err } - issuePr, baseRepo, err := issueShared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, fields()) - - parent := alias[opts.ParentCmd] - + baseRepo, err := opts.BaseRepo() if err != nil { return err - } else if parent.Typename != issuePr.Typename { + } + + issuePr, err := issueShared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, fields()) + if err != nil { + return err + } + + parent := alias[opts.ParentCmd] + if parent.Typename != issuePr.Typename { currentType := alias[parent.Typename] correctType := alias[issuePr.Typename] diff --git a/pkg/cmd/issue/lock/lock_test.go b/pkg/cmd/issue/lock/lock_test.go index f6dcb746d..1ca320f35 100644 --- a/pkg/cmd/issue/lock/lock_test.go +++ b/pkg/cmd/issue/lock/lock_test.go @@ -30,7 +30,7 @@ func Test_NewCmdLock(t *testing.T) { args: "--reason off_topic 451", want: LockOptions{ Reason: "off_topic", - SelectorArg: "451", + IssueNumber: 451, }, }, { @@ -41,9 +41,36 @@ func Test_NewCmdLock(t *testing.T) { name: "no flags", args: "451", want: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, }, }, + { + name: "issue number argument", + args: "451 --repo owner/repo", + want: LockOptions{ + IssueNumber: 451, + }, + }, + { + name: "argument is hash prefixed number", + // Escaping is required here to avoid what I think is shellex treating it as a comment. + args: "\\#451 --repo owner/repo", + want: LockOptions{ + IssueNumber: 451, + }, + }, + { + name: "argument is a URL", + args: "https://github.com/cli/cli/issues/451", + want: LockOptions{ + IssueNumber: 451, + }, + }, + { + name: "argument cannot be parsed to an issue", + args: "unparseable", + wantErr: "invalid issue format: \"unparseable\"", + }, { name: "bad reason", args: "--reason bad 451", @@ -60,7 +87,7 @@ func Test_NewCmdLock(t *testing.T) { args: "451", tty: true, want: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, Interactive: true, }, }, @@ -99,7 +126,7 @@ func Test_NewCmdLock(t *testing.T) { } assert.Equal(t, tt.want.Reason, opts.Reason) - assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg) + assert.Equal(t, tt.want.IssueNumber, opts.IssueNumber) assert.Equal(t, tt.want.Interactive, opts.Interactive) }) } @@ -121,9 +148,36 @@ func Test_NewCmdUnlock(t *testing.T) { name: "no flags", args: "451", want: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, }, }, + { + name: "issue number argument", + args: "451 --repo owner/repo", + want: LockOptions{ + IssueNumber: 451, + }, + }, + { + name: "argument is hash prefixed number", + // Escaping is required here to avoid what I think is shellex treating it as a comment. + args: "\\#451 --repo owner/repo", + want: LockOptions{ + IssueNumber: 451, + }, + }, + { + name: "argument is a URL", + args: "https://github.com/cli/cli/issues/451", + want: LockOptions{ + IssueNumber: 451, + }, + }, + { + name: "argument cannot be parsed to an issue", + args: "unparseable", + wantErr: "invalid issue format: \"unparseable\"", + }, } for _, tt := range cases { @@ -158,7 +212,7 @@ func Test_NewCmdUnlock(t *testing.T) { assert.NoError(t, err) } - assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg) + assert.Equal(t, tt.want.IssueNumber, opts.IssueNumber) }) } } @@ -179,7 +233,7 @@ func Test_runLock(t *testing.T) { name: "lock issue nontty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { @@ -203,7 +257,7 @@ func Test_runLock(t *testing.T) { tty: true, opts: LockOptions{ Interactive: true, - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", }, state: Lock, @@ -241,7 +295,7 @@ func Test_runLock(t *testing.T) { tty: true, state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", Reason: "off_topic", }, @@ -268,7 +322,7 @@ func Test_runLock(t *testing.T) { tty: true, state: Unlock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { @@ -294,7 +348,7 @@ func Test_runLock(t *testing.T) { name: "unlock issue nontty", state: Unlock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { @@ -319,7 +373,7 @@ func Test_runLock(t *testing.T) { name: "lock issue with explicit reason nontty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", Reason: "off_topic", }, @@ -344,7 +398,7 @@ func Test_runLock(t *testing.T) { name: "relock issue tty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", Reason: "off_topic", }, @@ -388,7 +442,7 @@ func Test_runLock(t *testing.T) { name: "relock issue nontty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "issue", Reason: "off_topic", }, @@ -409,7 +463,7 @@ func Test_runLock(t *testing.T) { name: "lock pr nontty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { @@ -433,7 +487,7 @@ func Test_runLock(t *testing.T) { tty: true, opts: LockOptions{ Interactive: true, - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", }, state: Lock, @@ -469,7 +523,7 @@ func Test_runLock(t *testing.T) { tty: true, state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", Reason: "off_topic", }, @@ -495,7 +549,7 @@ func Test_runLock(t *testing.T) { name: "lock pr with explicit nontty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", Reason: "off_topic", }, @@ -520,7 +574,7 @@ func Test_runLock(t *testing.T) { name: "unlock pr tty", state: Unlock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { @@ -546,7 +600,7 @@ func Test_runLock(t *testing.T) { tty: true, state: Unlock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { @@ -572,7 +626,7 @@ func Test_runLock(t *testing.T) { name: "relock pr tty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", Reason: "off_topic", }, @@ -616,7 +670,7 @@ func Test_runLock(t *testing.T) { name: "relock pr nontty", state: Lock, opts: LockOptions{ - SelectorArg: "451", + IssueNumber: 451, ParentCmd: "pr", Reason: "off_topic", }, diff --git a/pkg/cmd/issue/pin/pin.go b/pkg/cmd/issue/pin/pin.go index dfb11a881..290bec507 100644 --- a/pkg/cmd/issue/pin/pin.go +++ b/pkg/cmd/issue/pin/pin.go @@ -20,7 +20,7 @@ type PinOptions struct { Config func() (gh.Config, error) IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - SelectorArg string + IssueNumber int } func NewCmdPin(f *cmdutil.Factory, runF func(*PinOptions) error) *cobra.Command { @@ -51,8 +51,22 @@ func NewCmdPin(f *cmdutil.Factory, runF func(*PinOptions) error) *cobra.Command `), Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - opts.BaseRepo = f.BaseRepo - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err + } + + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber if runF != nil { return runF(opts) @@ -73,7 +87,12 @@ func pinRun(opts *PinOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "isPinned"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title", "isPinned"}) if err != nil { return err } diff --git a/pkg/cmd/issue/pin/pin_test.go b/pkg/cmd/issue/pin/pin_test.go index d4979a30d..67b767b32 100644 --- a/pkg/cmd/issue/pin/pin_test.go +++ b/pkg/cmd/issue/pin/pin_test.go @@ -1,80 +1,21 @@ package pin import ( - "bytes" "net/http" "testing" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/google/shlex" "github.com/stretchr/testify/assert" ) func TestNewCmdPin(t *testing.T) { - tests := []struct { - name string - input string - output PinOptions - wantErr bool - errMsg string - }{ - { - name: "no argument", - input: "", - wantErr: true, - errMsg: "accepts 1 arg(s), received 0", - }, - { - name: "issue number", - input: "6", - output: PinOptions{ - SelectorArg: "6", - }, - }, - { - name: "issue url", - input: "https://github.com/cli/cli/6", - output: PinOptions{ - SelectorArg: "https://github.com/cli/cli/6", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ios, _, _, _ := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - f := &cmdutil.Factory{ - IOStreams: ios, - } - argv, err := shlex.Split(tt.input) - assert.NoError(t, err) - var gotOpts *PinOptions - cmd := NewCmdPin(f, func(opts *PinOptions) error { - gotOpts = opts - return nil - }) - cmd.SetArgs(argv) - cmd.SetIn(&bytes.Buffer{}) - cmd.SetOut(&bytes.Buffer{}) - cmd.SetErr(&bytes.Buffer{}) - - _, err = cmd.ExecuteC() - if tt.wantErr { - assert.Error(t, err) - assert.Equal(t, tt.errMsg, err.Error()) - return - } - - assert.NoError(t, err) - assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg) - }) - } + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdPin) } func TestPinRun(t *testing.T) { @@ -89,7 +30,7 @@ func TestPinRun(t *testing.T) { { name: "pin issue", tty: true, - opts: &PinOptions{SelectorArg: "20"}, + opts: &PinOptions{IssueNumber: 20}, httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`query IssueByNumber\b`), @@ -113,7 +54,7 @@ func TestPinRun(t *testing.T) { { name: "issue already pinned", tty: true, - opts: &PinOptions{SelectorArg: "20"}, + opts: &PinOptions{IssueNumber: 20}, httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`query IssueByNumber\b`), diff --git a/pkg/cmd/issue/reopen/reopen.go b/pkg/cmd/issue/reopen/reopen.go index 92f18a7d9..f01a8eafc 100644 --- a/pkg/cmd/issue/reopen/reopen.go +++ b/pkg/cmd/issue/reopen/reopen.go @@ -21,7 +21,7 @@ type ReopenOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - SelectorArg string + IssueNumber int Comment string } @@ -37,13 +37,23 @@ func NewCmdReopen(f *cmdutil.Factory, runF func(*ReopenOptions) error) *cobra.Co Short: "Reopen issue", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo - - if len(args) > 0 { - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err } + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + if runF != nil { return runF(opts) } @@ -64,7 +74,12 @@ func reopenRun(opts *ReopenOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "state"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title", "state"}) if err != nil { return err } diff --git a/pkg/cmd/issue/reopen/reopen_test.go b/pkg/cmd/issue/reopen/reopen_test.go index 4b8b33ee1..f7c8cb95a 100644 --- a/pkg/cmd/issue/reopen/reopen_test.go +++ b/pkg/cmd/issue/reopen/reopen_test.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -18,6 +19,11 @@ import ( "github.com/stretchr/testify/assert" ) +func TestNewCmdReopen(t *testing.T) { + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdReopen) +} + func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(isTTY) diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index be79f9a73..5c477363b 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -13,69 +13,104 @@ import ( "github.com/cli/cli/v2/api" fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" + o "github.com/cli/cli/v2/pkg/option" "github.com/cli/cli/v2/pkg/set" "golang.org/x/sync/errgroup" ) -// IssueFromArgWithFields loads an issue or pull request with the specified fields. If some of the fields -// could not be fetched by GraphQL, this returns a non-nil issue and a *PartialLoadError. -func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, fields []string) (*api.Issue, ghrepo.Interface, error) { - issueNumber, baseRepo, err := IssueNumberAndRepoFromArg(arg) - if err != nil { - return nil, nil, err - } +var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/(?:issues|pull)/(\d+)`) - if baseRepo == nil { - var err error - if baseRepo, err = baseRepoFn(); err != nil { - return nil, nil, err - } - } +func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], error) { + var repo o.Option[ghrepo.Interface] + issueNumbers := make([]int, len(args)) - issue, err := findIssueOrPR(httpClient, baseRepo, issueNumber, fields) - return issue, baseRepo, err -} - -// IssuesFromArgsWithFields loads 1 or more issues or pull requests with the specified fields. If some of the fields -// could not be fetched by GraphQL, this returns non-nil issues and a *PartialLoadError. -func IssuesFromArgsWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), args []string, fields []string) ([]*api.Issue, ghrepo.Interface, error) { - var issuesRepo ghrepo.Interface - issueNumbers := make([]int, 0, len(args)) - - for _, arg := range args { - issueNumber, baseRepo, err := IssueNumberAndRepoFromArg(arg) + for i, arg := range args { + // For each argument, parse the issue number and an optional repo + issueNumber, issueRepo, err := ParseIssueFromArg(arg) if err != nil { - return nil, nil, err + return nil, o.None[ghrepo.Interface](), err } - issueNumbers = append(issueNumbers, issueNumber) - if baseRepo == nil { - var err error - if baseRepo, err = baseRepoFn(); err != nil { - return nil, nil, err + // if this is our first issue repo found, then we need to set it + if repo.IsNone() { + repo = issueRepo + } + + // if there is an issue repo returned, then we need to check if it is the same as the previous one + if issueRepo.IsSome() && repo.IsSome() { + // Unwraps are safe because we've checked for presence above + if !ghrepo.IsSame(repo.Unwrap(), issueRepo.Unwrap()) { + return nil, o.None[ghrepo.Interface](), fmt.Errorf( + "multiple issues must be in same repo: found %q, expected %q", + ghrepo.FullName(issueRepo.Unwrap()), + ghrepo.FullName(repo.Unwrap()), + ) } } - if issuesRepo == nil { - issuesRepo = baseRepo - continue - } - - if !ghrepo.IsSame(issuesRepo, baseRepo) { - return nil, nil, fmt.Errorf( - "multiple issues must be in same repo: found %q, expected %q", - ghrepo.FullName(baseRepo), - ghrepo.FullName(issuesRepo), - ) - } + // add the issue number to the list + issueNumbers[i] = issueNumber } - issuesChan := make(chan *api.Issue, len(args)) + return issueNumbers, repo, nil +} + +func ParseIssueFromArg(arg string) (int, o.Option[ghrepo.Interface], error) { + issueLocator := tryParseIssueFromURL(arg) + if issueLocator, present := issueLocator.Value(); present { + return issueLocator.issueNumber, o.Some(issueLocator.repo), nil + } + + issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")) + if err != nil { + return 0, o.None[ghrepo.Interface](), fmt.Errorf("invalid issue format: %q", arg) + } + + return issueNumber, o.None[ghrepo.Interface](), nil +} + +type issueLocator struct { + issueNumber int + repo ghrepo.Interface +} + +// tryParseIssueFromURL tries to parse an issue number and repo from a URL. +func tryParseIssueFromURL(maybeURL string) o.Option[issueLocator] { + u, err := url.Parse(maybeURL) + if err != nil { + return o.None[issueLocator]() + } + + if u.Scheme != "https" && u.Scheme != "http" { + return o.None[issueLocator]() + } + + m := issueURLRE.FindStringSubmatch(u.Path) + if m == nil { + return o.None[issueLocator]() + } + + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + issueNumber, _ := strconv.Atoi(m[3]) + return o.Some(issueLocator{ + issueNumber: issueNumber, + repo: repo, + }) +} + +type PartialLoadError struct { + error +} + +// FindIssuesOrPRs loads 1 or more issues or pull requests with the specified fields. If some of the fields +// could not be fetched by GraphQL, this returns non-nil issues and a *PartialLoadError. +func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumbers []int, fields []string) ([]*api.Issue, error) { + issuesChan := make(chan *api.Issue, len(issueNumbers)) g := errgroup.Group{} for _, num := range issueNumbers { issueNumber := num g.Go(func() error { - issue, err := findIssueOrPR(httpClient, issuesRepo, issueNumber, fields) + issue, err := FindIssueOrPR(httpClient, repo, issueNumber, fields) if err != nil { return err } @@ -89,60 +124,18 @@ func IssuesFromArgsWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo close(issuesChan) if err != nil { - return nil, nil, err + return nil, err } - issues := make([]*api.Issue, 0, len(args)) + issues := make([]*api.Issue, 0, len(issueNumbers)) for issue := range issuesChan { issues = append(issues, issue) } - return issues, issuesRepo, nil + return issues, nil } -var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/(?:issues|pull)/(\d+)`) - -func issueMetadataFromURL(s string) (int, ghrepo.Interface) { - u, err := url.Parse(s) - if err != nil { - return 0, nil - } - - if u.Scheme != "https" && u.Scheme != "http" { - return 0, nil - } - - m := issueURLRE.FindStringSubmatch(u.Path) - if m == nil { - return 0, nil - } - - repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) - issueNumber, _ := strconv.Atoi(m[3]) - return issueNumber, repo -} - -// Returns the issue number and repo if the issue URL is provided. -// If only the issue number is provided, returns the number and nil repo. -func IssueNumberAndRepoFromArg(arg string) (int, ghrepo.Interface, error) { - issueNumber, baseRepo := issueMetadataFromURL(arg) - - if issueNumber == 0 { - var err error - issueNumber, err = strconv.Atoi(strings.TrimPrefix(arg, "#")) - if err != nil { - return 0, nil, fmt.Errorf("invalid issue format: %q", arg) - } - } - - return issueNumber, baseRepo, nil -} - -type PartialLoadError struct { - error -} - -func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) { +func FindIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) { fieldSet := set.NewStringSet() fieldSet.AddValues(fields) if fieldSet.Contains("stateReason") { diff --git a/pkg/cmd/issue/shared/lookup_test.go b/pkg/cmd/issue/shared/lookup_test.go index 44f496de4..f921ca49b 100644 --- a/pkg/cmd/issue/shared/lookup_test.go +++ b/pkg/cmd/issue/shared/lookup_test.go @@ -2,265 +2,94 @@ package shared import ( "net/http" - "strings" "testing" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" + o "github.com/cli/cli/v2/pkg/option" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestIssueFromArgWithFields(t *testing.T) { - type args struct { - baseRepoFn func() (ghrepo.Interface, error) - selector string - } +func TestParseIssuesFromArgs(t *testing.T) { tests := []struct { - name string - args args - httpStub func(*httpmock.Registry) - wantIssue int - wantRepo string - wantProjects string - wantErr bool + behavior string + args []string + expectedIssueNumbers []int + expectedRepo o.Option[ghrepo.Interface] + expectedErr bool }{ { - name: "number argument", - args: args{ - selector: "13", - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") - }, - }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasIssuesEnabled": true, - "issue":{"number":13} - }}}`)) - }, - wantIssue: 13, - wantRepo: "https://github.com/OWNER/REPO", + behavior: "when given issue numbers, returns them with no repo", + args: []string{"1", "2"}, + expectedIssueNumbers: []int{1, 2}, + expectedRepo: o.None[ghrepo.Interface](), }, { - name: "number with hash argument", - args: args{ - selector: "#13", - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") - }, - }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasIssuesEnabled": true, - "issue":{"number":13} - }}}`)) - }, - wantIssue: 13, - wantRepo: "https://github.com/OWNER/REPO", + behavior: "when given # prefixed issue numbers, returns them with no repo", + args: []string{"#1", "#2"}, + expectedIssueNumbers: []int{1, 2}, + expectedRepo: o.None[ghrepo.Interface](), }, { - name: "URL argument", - args: args{ - selector: "https://example.org/OWNER/REPO/issues/13#comment-123", - baseRepoFn: nil, + behavior: "when given URLs, returns them with the repo", + args: []string{ + "https://github.com/OWNER/REPO/issues/1", + "https://github.com/OWNER/REPO/issues/2", }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasIssuesEnabled": true, - "issue":{"number":13} - }}}`)) - }, - wantIssue: 13, - wantRepo: "https://example.org/OWNER/REPO", + expectedIssueNumbers: []int{1, 2}, + expectedRepo: o.Some(ghrepo.New("OWNER", "REPO")), }, { - name: "PR URL argument", - args: args{ - selector: "https://example.org/OWNER/REPO/pull/13#comment-123", - baseRepoFn: nil, + behavior: "when given URLs in different repos, errors", + args: []string{ + "https://github.com/OWNER/REPO/issues/1", + "https://github.com/OWNER/OTHERREPO/issues/2", }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasIssuesEnabled": true, - "issue":{"number":13} - }}}`)) - }, - wantIssue: 13, - wantRepo: "https://example.org/OWNER/REPO", + expectedErr: true, }, { - name: "project cards permission issue", - args: args{ - selector: "https://example.org/OWNER/REPO/issues/13", - baseRepoFn: nil, - }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(` - { - "data": { - "repository": { - "hasIssuesEnabled": true, - "issue": { - "number": 13, - "projectCards": { - "nodes": [ - null, - { - "project": {"name": "myproject"}, - "column": {"name": "To Do"} - }, - null, - { - "project": {"name": "other project"}, - "column": null - } - ] - } - } - } - }, - "errors": [ - { - "type": "FORBIDDEN", - "message": "Resource not accessible by integration", - "path": ["repository", "issue", "projectCards", "nodes", 0] - }, - { - "type": "FORBIDDEN", - "message": "Resource not accessible by integration", - "path": ["repository", "issue", "projectCards", "nodes", 2] - } - ] - }`)) - }, - wantErr: true, - wantIssue: 13, - wantProjects: "myproject, other project", - wantRepo: "https://example.org/OWNER/REPO", + behavior: "when given an unparseable argument, errors", + args: []string{"://"}, + expectedErr: true, }, { - name: "projects permission issue", - args: args{ - selector: "https://example.org/OWNER/REPO/issues/13", - baseRepoFn: nil, - }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(` - { - "data": { - "repository": { - "hasIssuesEnabled": true, - "issue": { - "number": 13, - "projectCards": { - "nodes": null, - "totalCount": 0 - } - } - } - }, - "errors": [ - { - "type": "FORBIDDEN", - "message": "Resource not accessible by integration", - "path": ["repository", "issue", "projectCards", "nodes"] - } - ] - }`)) - }, - wantErr: true, - wantIssue: 13, - wantProjects: "", - wantRepo: "https://example.org/OWNER/REPO", + behavior: "when given a URL that isn't an issue or PR url, errors", + args: []string{"https://github.com"}, + expectedErr: true, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - reg := &httpmock.Registry{} - if tt.httpStub != nil { - tt.httpStub(reg) - } - httpClient := &http.Client{Transport: reg} - issue, repo, err := IssueFromArgWithFields(httpClient, tt.args.baseRepoFn, tt.args.selector, []string{"number"}) - if (err != nil) != tt.wantErr { - t.Errorf("IssueFromArgWithFields() error = %v, wantErr %v", err, tt.wantErr) - if issue == nil { - return - } - } - if issue.Number != tt.wantIssue { - t.Errorf("want issue #%d, got #%d", tt.wantIssue, issue.Number) - } - if gotProjects := strings.Join(issue.ProjectCards.ProjectNames(), ", "); gotProjects != tt.wantProjects { - t.Errorf("want projects %q, got %q", tt.wantProjects, gotProjects) - } - repoURL := ghrepo.GenerateRepoURL(repo, "") - if repoURL != tt.wantRepo { - t.Errorf("want repo %s, got %s", tt.wantRepo, repoURL) + + for _, tc := range tests { + t.Run(tc.behavior, func(t *testing.T) { + issueNumbers, repo, err := ParseIssuesFromArgs(tc.args) + + if tc.expectedErr { + require.Error(t, err) + return } + + require.NoError(t, err) + assert.Equal(t, tc.expectedIssueNumbers, issueNumbers) + assert.Equal(t, tc.expectedRepo, repo) }) } + } -func TestIssuesFromArgsWithFields(t *testing.T) { - type args struct { - baseRepoFn func() (ghrepo.Interface, error) - selectors []string - } +func TestFindIssuesOrPRs(t *testing.T) { tests := []struct { - name string - args args - httpStub func(*httpmock.Registry) - wantIssues []int - wantRepo string - wantErr bool - wantErrMsg string + name string + issueNumbers []int + baseRepo ghrepo.Interface + httpStub func(*httpmock.Registry) + wantIssueNumbers []int + wantErr bool }{ { - name: "multiple repos", - args: args{ - selectors: []string{"1", "https://github.com/OWNER/OTHERREPO/issues/2"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, - }, - httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasIssuesEnabled": true, - "issue":{"number":1} - }}}`)) - r.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(`{"data":{"repository":{ - "hasIssuesEnabled": true, - "issue":{"number":2} - }}}`)) - }, - wantErr: true, - wantErrMsg: "multiple issues must be in same repo", - }, - { - name: "multiple issues", - args: args{ - selectors: []string{"1", "2"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, - }, + name: "multiple issues", + issueNumbers: []int{1, 2}, + baseRepo: ghrepo.New("OWNER", "REPO"), httpStub: func(r *httpmock.Registry) { r.Register( httpmock.GraphQL(`query IssueByNumber\b`), @@ -275,43 +104,48 @@ func TestIssuesFromArgsWithFields(t *testing.T) { "issue":{"number":2} }}}`)) }, - wantIssues: []int{1, 2}, - wantRepo: "https://github.com/OWNER/REPO", + wantIssueNumbers: []int{1, 2}, + }, + { + name: "any find error results in total error", + issueNumbers: []int{1, 2}, + baseRepo: ghrepo.New("OWNER", "REPO"), + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query IssueByNumber\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "hasIssuesEnabled": true, + "issue":{"number":1} + }}}`)) + r.Register( + httpmock.GraphQL(`query IssueByNumber\b`), + httpmock.StatusStringResponse(500, "internal server error")) + }, + wantErr: true, }, } for _, tt := range tests { - if !tt.wantErr && len(tt.args.selectors) != len(tt.wantIssues) { - t.Fatal("number of selectors and issues not equal") - } t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} if tt.httpStub != nil { tt.httpStub(reg) } httpClient := &http.Client{Transport: reg} - issues, repo, err := IssuesFromArgsWithFields(httpClient, tt.args.baseRepoFn, tt.args.selectors, []string{"number"}) + issues, err := FindIssuesOrPRs(httpClient, tt.baseRepo, tt.issueNumbers, []string{"number"}) if (err != nil) != tt.wantErr { - t.Errorf("IssuesFromArgsWithFields() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("FindIssuesOrPRs() error = %v, wantErr %v", err, tt.wantErr) if issues == nil { return } } if tt.wantErr { - assert.Error(t, err) - assert.ErrorContains(t, err, tt.wantErrMsg) + require.Error(t, err) return } - assert.NoError(t, err) + + require.NoError(t, err) for i := range issues { - assert.Contains(t, tt.wantIssues, issues[i].Number) - } - if repo != nil { - repoURL := ghrepo.GenerateRepoURL(repo, "") - if repoURL != tt.wantRepo { - t.Errorf("want repo %s, got %s", tt.wantRepo, repoURL) - } - } else if tt.wantRepo != "" { - t.Errorf("want repo %sw, got nil", tt.wantRepo) + assert.Contains(t, tt.wantIssueNumbers, issues[i].Number) } }) } diff --git a/pkg/cmd/issue/transfer/transfer.go b/pkg/cmd/issue/transfer/transfer.go index 140d02b91..a6dfb9b23 100644 --- a/pkg/cmd/issue/transfer/transfer.go +++ b/pkg/cmd/issue/transfer/transfer.go @@ -20,7 +20,7 @@ type TransferOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - IssueSelector string + IssueNumber int DestRepoSelector string } @@ -36,8 +36,23 @@ func NewCmdTransfer(f *cmdutil.Factory, runF func(*TransferOptions) error) *cobr Short: "Transfer issue to another repository", Args: cmdutil.ExactArgs(2, "issue and destination repository are required"), RunE: func(cmd *cobra.Command, args []string) error { - opts.BaseRepo = f.BaseRepo - opts.IssueSelector = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err + } + + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + opts.DestRepoSelector = args[1] if runF != nil { @@ -57,7 +72,12 @@ func transferRun(opts *TransferOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.IssueSelector, []string{"id", "number"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number"}) if err != nil { return err } diff --git a/pkg/cmd/issue/transfer/transfer_test.go b/pkg/cmd/issue/transfer/transfer_test.go index eed9c5d85..2b12db944 100644 --- a/pkg/cmd/issue/transfer/transfer_test.go +++ b/pkg/cmd/issue/transfer/transfer_test.go @@ -15,6 +15,7 @@ import ( "github.com/cli/cli/v2/test" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func runCommand(rt http.RoundTripper, cli string) (*test.CmdOut, error) { @@ -57,18 +58,49 @@ func runCommand(rt http.RoundTripper, cli string) (*test.CmdOut, error) { func TestNewCmdTransfer(t *testing.T) { tests := []struct { - name string - cli string - wants TransferOptions - wantErr string + name string + cli string + wants TransferOptions + wantBaseRepo ghrepo.Interface + wantErr bool }{ { - name: "issue name", - cli: "3252 OWNER/REPO", + name: "no argument", + cli: "", + wantErr: true, + }, + { + name: "issue number argument", + cli: "--repo cli/repo 23 OWNER/REPO", wants: TransferOptions{ - IssueSelector: "3252", + IssueNumber: 23, DestRepoSelector: "OWNER/REPO", }, + wantBaseRepo: ghrepo.New("cli", "repo"), + }, + { + name: "argument is hash prefixed number", + // Escaping is required here to avoid what I think is shellex treating it as a comment. + cli: "--repo cli/repo \\#23 OWNER/REPO", + wants: TransferOptions{ + IssueNumber: 23, + DestRepoSelector: "OWNER/REPO", + }, + wantBaseRepo: ghrepo.New("cli", "repo"), + }, + { + name: "argument is a URL", + cli: "https://github.com/cli/cli/issues/23 OWNER/REPO", + wants: TransferOptions{ + IssueNumber: 23, + DestRepoSelector: "OWNER/REPO", + }, + wantBaseRepo: ghrepo.New("cli", "cli"), + }, + { + name: "argument cannot be parsed to an issue", + cli: "unparseable OWNER/REPO", + wantErr: true, }, } @@ -84,15 +116,29 @@ func TestNewCmdTransfer(t *testing.T) { gotOpts = opts return nil }) + cmdutil.EnableRepoOverride(cmd, f) + cmd.SetArgs(argv) cmd.SetIn(&bytes.Buffer{}) cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) _, cErr := cmd.ExecuteC() - assert.NoError(t, cErr) - assert.Equal(t, tt.wants.IssueSelector, gotOpts.IssueSelector) + if tt.wantErr { + require.Error(t, cErr) + return + } + + require.NoError(t, cErr) + assert.Equal(t, tt.wants.IssueNumber, gotOpts.IssueNumber) assert.Equal(t, tt.wants.DestRepoSelector, gotOpts.DestRepoSelector) + actualBaseRepo, err := gotOpts.BaseRepo() + require.NoError(t, err) + assert.True( + t, + ghrepo.IsSame(tt.wantBaseRepo, actualBaseRepo), + "expected base repo %+v, got %+v", tt.wantBaseRepo, actualBaseRepo, + ) }) } } diff --git a/pkg/cmd/issue/unpin/unpin.go b/pkg/cmd/issue/unpin/unpin.go index 3ac28d47c..ca22aa82e 100644 --- a/pkg/cmd/issue/unpin/unpin.go +++ b/pkg/cmd/issue/unpin/unpin.go @@ -16,11 +16,12 @@ import ( ) type UnpinOptions struct { - HttpClient func() (*http.Client, error) - Config func() (gh.Config, error) - IO *iostreams.IOStreams - BaseRepo func() (ghrepo.Interface, error) - SelectorArg string + HttpClient func() (*http.Client, error) + Config func() (gh.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + + IssueNumber int } func NewCmdUnpin(f *cmdutil.Factory, runF func(*UnpinOptions) error) *cobra.Command { @@ -51,8 +52,22 @@ func NewCmdUnpin(f *cmdutil.Factory, runF func(*UnpinOptions) error) *cobra.Comm `), Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - opts.BaseRepo = f.BaseRepo - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err + } + + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber if runF != nil { return runF(opts) @@ -73,7 +88,12 @@ func unpinRun(opts *UnpinOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "isPinned"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title", "isPinned"}) if err != nil { return err } diff --git a/pkg/cmd/issue/unpin/unpin_test.go b/pkg/cmd/issue/unpin/unpin_test.go index 70a018d94..3cdf29a74 100644 --- a/pkg/cmd/issue/unpin/unpin_test.go +++ b/pkg/cmd/issue/unpin/unpin_test.go @@ -1,80 +1,21 @@ package unpin import ( - "bytes" "net/http" "testing" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/google/shlex" "github.com/stretchr/testify/assert" ) -func TestNewCmdPin(t *testing.T) { - tests := []struct { - name string - input string - output UnpinOptions - wantErr bool - errMsg string - }{ - { - name: "no argument", - input: "", - wantErr: true, - errMsg: "accepts 1 arg(s), received 0", - }, - { - name: "issue number", - input: "6", - output: UnpinOptions{ - SelectorArg: "6", - }, - }, - { - name: "issue url", - input: "https://github.com/cli/cli/6", - output: UnpinOptions{ - SelectorArg: "https://github.com/cli/cli/6", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ios, _, _, _ := iostreams.Test() - ios.SetStdinTTY(true) - ios.SetStdoutTTY(true) - f := &cmdutil.Factory{ - IOStreams: ios, - } - argv, err := shlex.Split(tt.input) - assert.NoError(t, err) - var gotOpts *UnpinOptions - cmd := NewCmdUnpin(f, func(opts *UnpinOptions) error { - gotOpts = opts - return nil - }) - cmd.SetArgs(argv) - cmd.SetIn(&bytes.Buffer{}) - cmd.SetOut(&bytes.Buffer{}) - cmd.SetErr(&bytes.Buffer{}) - - _, err = cmd.ExecuteC() - if tt.wantErr { - assert.Error(t, err) - assert.Equal(t, tt.errMsg, err.Error()) - return - } - - assert.NoError(t, err) - assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg) - }) - } +func TestNewCmdUnpin(t *testing.T) { + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdUnpin) } func TestUnpinRun(t *testing.T) { @@ -89,7 +30,7 @@ func TestUnpinRun(t *testing.T) { { name: "unpin issue", tty: true, - opts: &UnpinOptions{SelectorArg: "20"}, + opts: &UnpinOptions{IssueNumber: 20}, httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`query IssueByNumber\b`), @@ -113,7 +54,7 @@ func TestUnpinRun(t *testing.T) { { name: "issue not pinned", tty: true, - opts: &UnpinOptions{SelectorArg: "20"}, + opts: &UnpinOptions{IssueNumber: 20}, httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`query IssueByNumber\b`), diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 8e3aa6040..f7838429b 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -14,6 +14,7 @@ import ( "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmd/issue/shared" issueShared "github.com/cli/cli/v2/pkg/cmd/issue/shared" prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -29,7 +30,7 @@ type ViewOptions struct { BaseRepo func() (ghrepo.Interface, error) Browser browser.Browser - SelectorArg string + IssueNumber int WebMode bool Comments bool Exporter cmdutil.Exporter @@ -55,13 +56,23 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman `, "`"), Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo - - if len(args) > 0 { - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err } + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + if runF != nil { return runF(opts) } @@ -87,6 +98,11 @@ func viewRun(opts *ViewOptions) error { return err } + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + lookupFields := set.NewStringSet() if opts.Exporter != nil { lookupFields.AddValues(opts.Exporter.Fields()) @@ -103,7 +119,18 @@ func viewRun(opts *ViewOptions) error { opts.IO.DetectTerminalTheme() opts.IO.StartProgressIndicator() - issue, baseRepo, err := findIssue(httpClient, opts.BaseRepo, opts.SelectorArg, lookupFields.ToSlice()) + lookupFields.Add("id") + + issue, err := issueShared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, lookupFields.ToSlice()) + if err != nil { + return err + } + + if lookupFields.Contains("comments") { + // FIXME: this re-fetches the comments connection even though the initial set of 100 were + // fetched in the previous request. + err = preloadIssueComments(httpClient, baseRepo, issue) + } opts.IO.StopProgressIndicator() if err != nil { var loadErr *issueShared.PartialLoadError @@ -143,24 +170,6 @@ func viewRun(opts *ViewOptions) error { return printRawIssuePreview(opts.IO.Out, issue) } -func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error), selector string, fields []string) (*api.Issue, ghrepo.Interface, error) { - fieldSet := set.NewStringSet() - fieldSet.AddValues(fields) - fieldSet.Add("id") - - issue, repo, err := issueShared.IssueFromArgWithFields(client, baseRepoFn, selector, fieldSet.ToSlice()) - if err != nil { - return issue, repo, err - } - - if fieldSet.Contains("comments") { - // FIXME: this re-fetches the comments connection even though the initial set of 100 were - // fetched in the previous request. - err = preloadIssueComments(client, repo, issue) - } - return issue, repo, err -} - func printRawIssuePreview(out io.Writer, issue *api.Issue) error { assignees := issueAssigneeList(*issue) labels := issueLabelList(issue, nil) diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index e1798af9f..2dd963687 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -13,6 +13,7 @@ import ( "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/run" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -47,6 +48,11 @@ func TestJSONFields(t *testing.T) { }) } +func TestNewCmdView(t *testing.T) { + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdView) +} + func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(isTTY) @@ -116,7 +122,7 @@ func TestIssueView_web(t *testing.T) { return ghrepo.New("OWNER", "REPO"), nil }, WebMode: true, - SelectorArg: "123", + IssueNumber: 123, }) if err != nil { t.Errorf("error running command `issue view`: %v", err) @@ -273,7 +279,7 @@ func TestIssueView_tty_Preview(t *testing.T) { BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, - SelectorArg: "123", + IssueNumber: 123, } err := viewRun(&opts)