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", },