Skip to content

Commit 7009321

Browse files
vilmibmvilmibm
committed
review feedback
1 parent c8e9768 commit 7009321

File tree

4 files changed

+76
-33
lines changed

4 files changed

+76
-33
lines changed

‎command/pr_review.go‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ import (
1515
)
1616

1717
func init() {
18-
// TODO re-register post release
19-
// prCmd.AddCommand(prReviewCmd)
18+
prCmd.AddCommand(prReviewCmd)
2019

2120
prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request")
2221
prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request")
@@ -62,17 +61,19 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
6261
state = api.ReviewComment
6362
}
6463

65-
if found == 0 {
66-
return nil, nil // signal interactive mode
67-
} else if found > 1 {
68-
return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")
69-
}
70-
7164
body, err := cmd.Flags().GetString("body")
7265
if err != nil {
7366
return nil, err
7467
}
7568

69+
if found == 0 && body == "" {
70+
return nil, nil // signal interactive mode
71+
} else if found == 0 && body != "" {
72+
return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment")
73+
} else if found > 1 {
74+
return nil, errors.New("need exactly one of --approve, --request-changes, or --comment")
75+
}
76+
7677
if (flag == "request-changes" || flag == "comment") && body == "" {
7778
return nil, fmt.Errorf("body cannot be blank for %s review", flag)
7879
}
@@ -116,7 +117,7 @@ func prReview(cmd *cobra.Command, args []string) error {
116117
}
117118
}
118119

119-
input, err := processReviewOpt(cmd)
120+
reviewData, err := processReviewOpt(cmd)
120121
if err != nil {
121122
return fmt.Errorf("did not understand desired review action: %w", err)
122123
}
@@ -132,24 +133,36 @@ func prReview(cmd *cobra.Command, args []string) error {
132133
if err != nil {
133134
return fmt.Errorf("could not find pull request: %w", err)
134135
}
136+
prNum = pr.Number
135137
}
136138

137-
if input == nil {
138-
input, err = reviewSurvey(cmd)
139+
out := colorableOut(cmd)
140+
141+
if reviewData == nil {
142+
reviewData, err = reviewSurvey(cmd)
139143
if err != nil {
140144
return err
141145
}
142-
if input == nil && err == nil {
143-
// Cancelled.
146+
if reviewData == nil && err == nil {
147+
fmt.Fprint(out, "Discarding.\n")
144148
return nil
145149
}
146150
}
147151

148-
err = api.AddReview(apiClient, pr, input)
152+
err = api.AddReview(apiClient, pr, reviewData)
149153
if err != nil {
150154
return fmt.Errorf("failed to create review: %w", err)
151155
}
152156

157+
switch reviewData.State {
158+
case api.ReviewComment:
159+
fmt.Fprintf(out, "%s Reviewed pull request #%d\n", utils.Gray("-"), prNum)
160+
case api.ReviewApprove:
161+
fmt.Fprintf(out, "%s Approved pull request #%d\n", utils.Green("✓"), prNum)
162+
case api.ReviewRequestChanges:
163+
fmt.Fprintf(out, "%s Requested changes to pull request #%d\n", utils.Red("+"), prNum)
164+
}
165+
153166
return nil
154167
}
155168

@@ -171,7 +184,6 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
171184
"Comment",
172185
"Approve",
173186
"Request changes",
174-
"Cancel",
175187
},
176188
},
177189
},
@@ -189,18 +201,22 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) {
189201
reviewState = api.ReviewApprove
190202
case "Request Changes":
191203
reviewState = api.ReviewRequestChanges
192-
case "Cancel":
193-
return nil, nil
194204
}
195205

196206
bodyAnswers := struct {
197207
Body string
198208
}{}
199209

210+
blankAllowed := false
211+
if reviewState == api.ReviewApprove {
212+
blankAllowed = true
213+
}
214+
200215
bodyQs := []*survey.Question{
201216
&survey.Question{
202217
Name: "body",
203218
Prompt: &surveyext.GhEditor{
219+
BlankAllowed: blankAllowed,
204220
EditorCommand: editorCommand,
205221
Editor: &survey.Editor{
206222
Message: "Review body",

‎command/pr_review_test.go‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
)
1212

1313
func TestPRReview_validation(t *testing.T) {
14-
t.Skip("skipping until release is done")
1514
initBlankContext("", "OWNER/REPO", "master")
1615
http := initFakeHTTP()
1716
for _, cmd := range []string{
@@ -20,12 +19,25 @@ func TestPRReview_validation(t *testing.T) {
2019
} {
2120
http.StubRepoResponse("OWNER", "REPO")
2221
_, err := RunCommand(cmd)
22+
if err == nil {
23+
t.Fatal("expected error")
24+
}
2325
eq(t, err.Error(), "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment")
2426
}
2527
}
2628

29+
func TestPRReview_bad_body(t *testing.T) {
30+
initBlankContext("", "OWNER/REPO", "master")
31+
http := initFakeHTTP()
32+
http.StubRepoResponse("OWNER", "REPO")
33+
_, err := RunCommand(`pr review -b "radical"`)
34+
if err == nil {
35+
t.Fatal("expected error")
36+
}
37+
eq(t, err.Error(), "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment")
38+
}
39+
2740
func TestPRReview_url_arg(t *testing.T) {
28-
t.Skip("skipping until release is done")
2941
initBlankContext("", "OWNER/REPO", "master")
3042
http := initFakeHTTP()
3143
http.StubRepoResponse("OWNER", "REPO")
@@ -48,11 +60,13 @@ func TestPRReview_url_arg(t *testing.T) {
4860
} } } } `))
4961
http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`))
5062

51-
_, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123")
63+
output, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123")
5264
if err != nil {
5365
t.Fatalf("error running pr review: %s", err)
5466
}
5567

68+
test.ExpectLines(t, output.String(), "Approved pull request #123")
69+
5670
bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
5771
reqBody := struct {
5872
Variables struct {
@@ -71,7 +85,6 @@ func TestPRReview_url_arg(t *testing.T) {
7185
}
7286

7387
func TestPRReview_number_arg(t *testing.T) {
74-
t.Skip("skipping until release is done")
7588
initBlankContext("", "OWNER/REPO", "master")
7689
http := initFakeHTTP()
7790
http.StubRepoResponse("OWNER", "REPO")
@@ -94,11 +107,13 @@ func TestPRReview_number_arg(t *testing.T) {
94107
} } } } `))
95108
http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`))
96109

97-
_, err := RunCommand("pr review --approve 123")
110+
output, err := RunCommand("pr review --approve 123")
98111
if err != nil {
99112
t.Fatalf("error running pr review: %s", err)
100113
}
101114

115+
test.ExpectLines(t, output.String(), "Approved pull request #123")
116+
102117
bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
103118
reqBody := struct {
104119
Variables struct {
@@ -117,24 +132,26 @@ func TestPRReview_number_arg(t *testing.T) {
117132
}
118133

119134
func TestPRReview_no_arg(t *testing.T) {
120-
t.Skip("skipping until release is done")
121135
initBlankContext("", "OWNER/REPO", "feature")
122136
http := initFakeHTTP()
123137
http.StubRepoResponse("OWNER", "REPO")
124138
http.StubResponse(200, bytes.NewBufferString(`
125139
{ "data": { "repository": { "pullRequests": { "nodes": [
126140
{ "url": "https://github.com/OWNER/REPO/pull/123",
141+
"number": 123,
127142
"id": "foobar123",
128143
"headRefName": "feature",
129144
"baseRefName": "master" }
130145
] } } } }`))
131146
http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`))
132147

133-
_, err := RunCommand(`pr review --comment -b "cool story"`)
148+
output, err := RunCommand(`pr review --comment -b "cool story"`)
134149
if err != nil {
135150
t.Fatalf("error running pr review: %s", err)
136151
}
137152

153+
test.ExpectLines(t, output.String(), "- Reviewed pull request #123")
154+
138155
bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
139156
reqBody := struct {
140157
Variables struct {
@@ -153,7 +170,6 @@ func TestPRReview_no_arg(t *testing.T) {
153170
}
154171

155172
func TestPRReview_blank_comment(t *testing.T) {
156-
t.Skip("skipping until release is done")
157173
initBlankContext("", "OWNER/REPO", "master")
158174
http := initFakeHTTP()
159175
http.StubRepoResponse("OWNER", "REPO")
@@ -163,7 +179,6 @@ func TestPRReview_blank_comment(t *testing.T) {
163179
}
164180

165181
func TestPRReview_blank_request_changes(t *testing.T) {
166-
t.Skip("skipping until release is done")
167182
initBlankContext("", "OWNER/REPO", "master")
168183
http := initFakeHTTP()
169184
http.StubRepoResponse("OWNER", "REPO")
@@ -173,7 +188,6 @@ func TestPRReview_blank_request_changes(t *testing.T) {
173188
}
174189

175190
func TestPRReview(t *testing.T) {
176-
t.Skip("skipping until release is done")
177191
type c struct {
178192
Cmd string
179193
ExpectedEvent string
@@ -228,6 +242,7 @@ func TestPRReview_interactive(t *testing.T) {
228242
http.StubResponse(200, bytes.NewBufferString(`
229243
{ "data": { "repository": { "pullRequests": { "nodes": [
230244
{ "url": "https://github.com/OWNER/REPO/pull/123",
245+
"number": 123,
231246
"id": "foobar123",
232247
"headRefName": "feature",
233248
"baseRefName": "master" }
@@ -262,6 +277,7 @@ func TestPRReview_interactive(t *testing.T) {
262277
}
263278

264279
test.ExpectLines(t, output.String(),
280+
"Approved pull request #123",
265281
"Got:",
266282
"cool.*story") // weird because markdown rendering puts a bunch of junk between works
267283

@@ -329,6 +345,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) {
329345
http.StubResponse(200, bytes.NewBufferString(`
330346
{ "data": { "repository": { "pullRequests": { "nodes": [
331347
{ "url": "https://github.com/OWNER/REPO/pull/123",
348+
"number": 123,
332349
"id": "foobar123",
333350
"headRefName": "feature",
334351
"baseRefName": "master" }
@@ -367,6 +384,8 @@ func TestPRReview_interactive_blank_approve(t *testing.T) {
367384
t.Errorf("did not expect to see body printed in %s", output.String())
368385
}
369386

387+
test.ExpectLines(t, output.String(), "Approved pull request #123")
388+
370389
bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
371390
reqBody := struct {
372391
Variables struct {

‎command/title_body_survey.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie
176176
qs = append(qs, bodyQuestion)
177177
}
178178

179-
err := SurveyAsk(qs, issueState)
179+
err = SurveyAsk(qs, issueState)
180180
if err != nil {
181181
return fmt.Errorf("could not prompt: %w", err)
182182
}

‎pkg/surveyext/editor.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ func init() {
3838
type GhEditor struct {
3939
*survey.Editor
4040
EditorCommand string
41+
BlankAllowed bool
4142
}
4243

4344
func (e *GhEditor) editorCommand() string {
@@ -58,13 +59,14 @@ var EditorQuestionTemplate = `
5859
{{- else }}
5960
{{- if and .Help (not .ShowHelp)}}{{color "cyan"}}[{{ .Config.HelpInput }} for help]{{color "reset"}} {{end}}
6061
{{- if and .Default (not .HideDefault)}}{{color "white"}}({{.Default}}) {{color "reset"}}{{end}}
61-
{{- color "cyan"}}[(e) to launch {{ .EditorCommand }}, enter to skip] {{color "reset"}}
62+
{{- color "cyan"}}[(e) to launch {{ .EditorCommand }}{{- if .BlankAllowed }}, enter to skip{{ end }}] {{color "reset"}}
6263
{{- end}}`
6364

6465
// EXTENDED to pass editor name (to use in prompt)
6566
type EditorTemplateData struct {
6667
survey.Editor
6768
EditorCommand string
69+
BlankAllowed bool
6870
Answer string
6971
ShowAnswer bool
7072
ShowHelp bool
@@ -75,9 +77,10 @@ type EditorTemplateData struct {
7577
func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (interface{}, error) {
7678
err := e.Render(
7779
EditorQuestionTemplate,
78-
// EXTENDED to support printing editor in prompt
80+
// EXTENDED to support printing editor in prompt and BlankAllowed
7981
EditorTemplateData{
8082
Editor: *e.Editor,
83+
BlankAllowed: e.BlankAllowed,
8184
EditorCommand: filepath.Base(e.editorCommand()),
8285
Config: config,
8386
},
@@ -96,7 +99,7 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int
9699
defer cursor.Show()
97100

98101
for {
99-
// EXTENDED to handle the e to edit / enter to skip behavior
102+
// EXTENDED to handle the e to edit / enter to skip behavior + BlankAllowed
100103
r, _, err := rr.ReadRune()
101104
if err != nil {
102105
return "", err
@@ -105,7 +108,11 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int
105108
break
106109
}
107110
if r == '\r' || r == '\n' {
108-
return "", nil
111+
if e.BlankAllowed {
112+
return "", nil
113+
} else {
114+
continue
115+
}
109116
}
110117
if r == terminal.KeyInterrupt {
111118
return "", terminal.InterruptErr
@@ -117,8 +124,9 @@ func (e *GhEditor) prompt(initialValue string, config *survey.PromptConfig) (int
117124
err = e.Render(
118125
EditorQuestionTemplate,
119126
EditorTemplateData{
120-
// EXTENDED to support printing editor in prompt
127+
// EXTENDED to support printing editor in prompt, BlankAllowed
121128
Editor: *e.Editor,
129+
BlankAllowed: e.BlankAllowed,
122130
EditorCommand: filepath.Base(e.editorCommand()),
123131
ShowHelp: true,
124132
Config: config,

0 commit comments

Comments
 (0)