Skip to content

Commit 12637d0

Browse files
mislavmislav
committed
Isolate pr review command
1 parent 3c32a84 commit 12637d0

File tree

5 files changed

+332
-242
lines changed

5 files changed

+332
-242
lines changed

‎command/root.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@ import (
1616
"github.com/MakeNowJust/heredoc"
1717
"github.com/cli/cli/api"
1818
"github.com/cli/cli/context"
19+
"github.com/cli/cli/git"
1920
"github.com/cli/cli/internal/config"
2021
"github.com/cli/cli/internal/ghrepo"
2122
"github.com/cli/cli/internal/run"
2223
apiCmd "github.com/cli/cli/pkg/cmd/api"
2324
gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create"
25+
prReviewCmd "github.com/cli/cli/pkg/cmd/pr/review"
2426
repoCmd "github.com/cli/cli/pkg/cmd/repo"
2527
repoCloneCmd "github.com/cli/cli/pkg/cmd/repo/clone"
2628
repoCreateCmd "github.com/cli/cli/pkg/cmd/repo/create"
@@ -113,6 +115,13 @@ func init() {
113115
}
114116
return cfg, nil
115117
},
118+
Branch: func() (string, error) {
119+
currentBranch, err := git.CurrentBranch()
120+
if err != nil {
121+
return "", fmt.Errorf("could not determine current branch: %w", err)
122+
}
123+
return currentBranch, nil
124+
},
116125
}
117126
RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil))
118127

@@ -160,6 +169,8 @@ func init() {
160169
repoCmd.Cmd.AddCommand(repoCreateCmd.NewCmdCreate(cmdFactory, nil))
161170
repoCmd.Cmd.AddCommand(creditsCmd.NewCmdRepoCredits(&repoResolvingCmdFactory, nil))
162171

172+
prCmd.AddCommand(prReviewCmd.NewCmdReview(&repoResolvingCmdFactory, nil))
173+
163174
RootCmd.AddCommand(creditsCmd.NewCmdCredits(cmdFactory, nil))
164175
}
165176

@@ -401,6 +412,7 @@ func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string {
401412
return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName())
402413
}
403414

415+
// TODO there is a parallel implementation for isolated commands
404416
func determineEditor(cmd *cobra.Command) (string, error) {
405417
editorCommand := os.Getenv("GH_EDITOR")
406418
if editorCommand == "" {

command/pr_review.go renamed to pkg/cmd/pr/review/review.go

Lines changed: 130 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -1,124 +1,120 @@
1-
package command
1+
package review
22

33
import (
44
"errors"
55
"fmt"
6+
"net/http"
67

78
"github.com/AlecAivazis/survey/v2"
89
"github.com/MakeNowJust/heredoc"
9-
"github.com/spf13/cobra"
10-
1110
"github.com/cli/cli/api"
11+
"github.com/cli/cli/context"
12+
"github.com/cli/cli/internal/config"
13+
"github.com/cli/cli/internal/ghrepo"
14+
"github.com/cli/cli/pkg/cmd/pr/shared"
15+
"github.com/cli/cli/pkg/cmdutil"
16+
"github.com/cli/cli/pkg/iostreams"
1217
"github.com/cli/cli/pkg/prompt"
1318
"github.com/cli/cli/pkg/surveyext"
1419
"github.com/cli/cli/utils"
20+
"github.com/spf13/cobra"
1521
)
1622

17-
func init() {
18-
prCmd.AddCommand(prReviewCmd)
19-
20-
prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request")
21-
prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request")
22-
prReviewCmd.Flags().BoolP("comment", "c", false, "Comment on a pull request")
23-
prReviewCmd.Flags().StringP("body", "b", "", "Specify the body of a review")
24-
}
25-
26-
var prReviewCmd = &cobra.Command{
27-
Use: "review [<number> | <url> | <branch>]",
28-
Short: "Add a review to a pull request",
29-
Long: `Add a review to a pull request.
30-
31-
Without an argument, the pull request that belongs to the current branch is reviewed.`,
32-
Example: heredoc.Doc(`
33-
# approve the pull request of the current branch
34-
$ gh pr review --approve
35-
36-
# leave a review comment for the current branch
37-
$ gh pr review --comment -b "interesting"
38-
39-
# add a review for a specific pull request
40-
$ gh pr review 123
41-
42-
# request changes on a specific pull request
43-
$ gh pr review 123 -r -b "needs more ASCII art"
44-
`),
45-
Args: cobra.MaximumNArgs(1),
46-
RunE: prReview,
23+
type ReviewOptions struct {
24+
HttpClient func() (*http.Client, error)
25+
Config func() (config.Config, error)
26+
IO *iostreams.IOStreams
27+
BaseRepo func() (ghrepo.Interface, error)
28+
Remotes func() (context.Remotes, error)
29+
Branch func() (string, error)
30+
31+
SelectorArg string
32+
Approve bool
33+
RequestChanges bool
34+
Comment bool
35+
Body string
4736
}
4837

49-
func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
50-
found := 0
51-
flag := ""
52-
var state api.PullRequestReviewState
53-
54-
if cmd.Flags().Changed("approve") {
55-
found++
56-
flag = "approve"
57-
state = api.ReviewApprove
58-
}
59-
if cmd.Flags().Changed("request-changes") {
60-
found++
61-
flag = "request-changes"
62-
state = api.ReviewRequestChanges
63-
}
64-
if cmd.Flags().Changed("comment") {
65-
found++
66-
flag = "comment"
67-
state = api.ReviewComment
68-
}
69-
70-
body, err := cmd.Flags().GetString("body")
71-
if err != nil {
72-
return nil, err
38+
func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Command {
39+
opts := &ReviewOptions{
40+
IO: f.IOStreams,
41+
HttpClient: f.HttpClient,
42+
Config: f.Config,
43+
BaseRepo: f.BaseRepo,
44+
Remotes: f.Remotes,
45+
Branch: f.Branch,
7346
}
7447

75-
if found == 0 && body == "" {
76-
if connectedToTerminal(cmd) {
77-
return nil, nil // signal interactive mode
78-
}
79-
return nil, errors.New("--approve, --request-changes, or --comment required when not attached to a tty")
80-
} else if found == 0 && body != "" {
81-
return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment")
82-
} else if found > 1 {
83-
return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")
48+
cmd := &cobra.Command{
49+
Use: "review [<number> | <url> | <branch>]",
50+
Short: "Add a review to a pull request",
51+
Long: heredoc.Doc(`
52+
Add a review to a pull request.
53+
54+
Without an argument, the pull request that belongs to the current branch is reviewed.
55+
`),
56+
Example: heredoc.Doc(`
57+
# approve the pull request of the current branch
58+
$ gh pr review --approve
59+
60+
# leave a review comment for the current branch
61+
$ gh pr review --comment -b "interesting"
62+
63+
# add a review for a specific pull request
64+
$ gh pr review 123
65+
66+
# request changes on a specific pull request
67+
$ gh pr review 123 -r -b "needs more ASCII art"
68+
`),
69+
Args: cobra.MaximumNArgs(1),
70+
RunE: func(cmd *cobra.Command, args []string) error {
71+
if len(args) > 0 {
72+
opts.SelectorArg = args[0]
73+
}
74+
75+
if runF != nil {
76+
return runF(opts)
77+
}
78+
return reviewRun(opts)
79+
},
8480
}
8581

86-
if (flag == "request-changes" || flag == "comment") && body == "" {
87-
return nil, fmt.Errorf("body cannot be blank for %s review", flag)
88-
}
82+
cmd.Flags().BoolVarP(&opts.Approve, "approve", "a", false, "Approve pull request")
83+
cmd.Flags().BoolVarP(&opts.RequestChanges, "request-changes", "r", false, "Request changes on a pull request")
84+
cmd.Flags().BoolVarP(&opts.Comment, "comment", "c", false, "Comment on a pull request")
85+
cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Specify the body of a review")
8986

90-
return &api.PullRequestReviewInput{
91-
Body: body,
92-
State: state,
93-
}, nil
87+
return cmd
9488
}
9589

96-
func prReview(cmd *cobra.Command, args []string) error {
97-
ctx := contextForCommand(cmd)
98-
apiClient, err := apiClientForContext(ctx)
90+
func reviewRun(opts *ReviewOptions) error {
91+
httpClient, err := opts.HttpClient()
9992
if err != nil {
10093
return err
10194
}
95+
apiClient := api.NewClientFromHTTP(httpClient)
10296

103-
pr, _, err := prFromArgs(ctx, apiClient, cmd, args)
97+
reviewData, err := processReviewOpt(opts)
10498
if err != nil {
105-
return err
99+
return fmt.Errorf("did not understand desired review action: %w", err)
106100
}
107101

108-
reviewData, err := processReviewOpt(cmd)
102+
pr, _, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg)
109103
if err != nil {
110-
return fmt.Errorf("did not understand desired review action: %w", err)
104+
return err
111105
}
112106

113-
stderr := colorableErr(cmd)
114-
115107
if reviewData == nil {
116-
reviewData, err = reviewSurvey(cmd)
108+
editorCommand, err := cmdutil.DetermineEditor(opts.Config)
109+
if err != nil {
110+
return err
111+
}
112+
reviewData, err = reviewSurvey(opts.IO, editorCommand)
117113
if err != nil {
118114
return err
119115
}
120116
if reviewData == nil && err == nil {
121-
fmt.Fprint(stderr, "Discarding.\n")
117+
fmt.Fprint(opts.IO.ErrOut, "Discarding.\n")
122118
return nil
123119
}
124120
}
@@ -128,28 +124,68 @@ func prReview(cmd *cobra.Command, args []string) error {
128124
return fmt.Errorf("failed to create review: %w", err)
129125
}
130126

131-
if !connectedToTerminal(cmd) {
127+
if !opts.IO.IsStdoutTTY() || !opts.IO.IsStderrTTY() {
132128
return nil
133129
}
134130

135131
switch reviewData.State {
136132
case api.ReviewComment:
137-
fmt.Fprintf(stderr, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number)
133+
fmt.Fprintf(opts.IO.ErrOut, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number)
138134
case api.ReviewApprove:
139-
fmt.Fprintf(stderr, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number)
135+
fmt.Fprintf(opts.IO.ErrOut, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number)
140136
case api.ReviewRequestChanges:
141-
fmt.Fprintf(stderr, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number)
137+
fmt.Fprintf(opts.IO.ErrOut, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number)
142138
}
143139

144140
return nil
145141
}
146142

147-
func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
148-
editorCommand, err := determineEditor(cmd)
149-
if err != nil {
150-
return nil, err
143+
// TODO: move to Command.Args, raise FlagError
144+
func processReviewOpt(opts *ReviewOptions) (*api.PullRequestReviewInput, error) {
145+
found := 0
146+
flag := ""
147+
var state api.PullRequestReviewState
148+
149+
if opts.Approve {
150+
found++
151+
flag = "approve"
152+
state = api.ReviewApprove
153+
}
154+
if opts.RequestChanges {
155+
found++
156+
flag = "request-changes"
157+
state = api.ReviewRequestChanges
158+
}
159+
if opts.Comment {
160+
found++
161+
flag = "comment"
162+
state = api.ReviewComment
151163
}
152164

165+
body := opts.Body
166+
167+
if found == 0 && body == "" {
168+
if opts.IO.IsStdoutTTY() && opts.IO.IsStderrTTY() {
169+
return nil, nil // signal interactive mode
170+
}
171+
return nil, errors.New("--approve, --request-changes, or --comment required when not attached to a tty")
172+
} else if found == 0 && body != "" {
173+
return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment")
174+
} else if found > 1 {
175+
return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")
176+
}
177+
178+
if (flag == "request-changes" || flag == "comment") && body == "" {
179+
return nil, fmt.Errorf("body cannot be blank for %s review", flag)
180+
}
181+
182+
return &api.PullRequestReviewInput{
183+
Body: body,
184+
State: state,
185+
}, nil
186+
}
187+
188+
func reviewSurvey(io *iostreams.IOStreams, editorCommand string) (*api.PullRequestReviewInput, error) {
153189
typeAnswers := struct {
154190
ReviewType string
155191
}{}
@@ -167,7 +203,7 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
167203
},
168204
}
169205

170-
err = prompt.SurveyAsk(typeQs, &typeAnswers)
206+
err := prompt.SurveyAsk(typeQs, &typeAnswers)
171207
if err != nil {
172208
return nil, err
173209
}
@@ -218,13 +254,12 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
218254
}
219255

220256
if len(bodyAnswers.Body) > 0 {
221-
out := colorableOut(cmd)
222257
renderedBody, err := utils.RenderMarkdown(bodyAnswers.Body)
223258
if err != nil {
224259
return nil, err
225260
}
226261

227-
fmt.Fprintf(out, "Got:\n%s", renderedBody)
262+
fmt.Fprintf(io.Out, "Got:\n%s", renderedBody)
228263
}
229264

230265
confirm := false

0 commit comments

Comments
 (0)