Repository: google/go-github
Pull request #1566 (2020-06-21 22:14:53)

Add support for multi-line comments.
This adds support for the "comfort-fade" preview, which enables multi-line comments (incl. suggested edits).

I have been carrying a subset of this as a patch downstream for a while, which just hard wired things on, but I tried to make this "smart" for back-compat as part of upstreaming.

codecov[bot]: # [Codecov](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=h1) Report
> Merging [#1566](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=desc) into [master](https://codecov.io/gh/google/go-github/commit/be9fac62b9268b22ffcf0ea430ddd5736fb499e7&el=desc) will **increase** coverage by `5.17%`.
> The diff coverage is `73.91%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-github/pull/1566/graphs/tree.svg?width=650&height=150&src=pr&token=EgYjGjZlI0)](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1566      +/-   ##
==========================================
+ Coverage   67.90%   73.08%   +5.17%     
==========================================
  Files          95       95              
  Lines        8722     7041    -1681     
==========================================
- Hits         5923     5146     -777     
+ Misses       1892      985     -907     
- Partials      907      910       +3     
```


| [Impacted Files](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [github/pulls\_reviews.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL3B1bGxzX3Jldmlld3MuZ28=) | `76.66% <73.91%> (+3.93%)` | :arrow_up: |
| [github/github.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL2dpdGh1Yi5nbw==) | `89.65% <0.00%> (-0.03%)` | :arrow_down: |
| [github/admin.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL2FkbWluLmdv) | `100.00% <0.00%> (ø)` | |
| [github/strings.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL3N0cmluZ3MuZ28=) | `100.00% <0.00%> (ø)` | |
| [github/activity.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL2FjdGl2aXR5Lmdv) | `100.00% <0.00%> (ø)` | |
| [github/timestamp.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL3RpbWVzdGFtcC5nbw==) | `100.00% <0.00%> (ø)` | |
| [github/admin\_stats.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL2FkbWluX3N0YXRzLmdv) | `100.00% <0.00%> (ø)` | |
| [github/repos\_traffic.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL3JlcG9zX3RyYWZmaWMuZ28=) | `100.00% <0.00%> (ø)` | |
| [github/pulls\_reviewers.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL3B1bGxzX3Jldmlld2Vycy5nbw==) | `100.00% <0.00%> (ø)` | |
| [github/git\_commits.go](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree#diff-Z2l0aHViL2dpdF9jb21taXRzLmdv) | `91.30% <0.00%> (+1.30%)` | :arrow_up: |
| ... and [83 more](https://codecov.io/gh/google/go-github/pull/1566/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=footer). Last update [be9fac6...e173bb2](https://codecov.io/gh/google/go-github/pull/1566?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

mattmoor: Yeah, ack on the docs.  I spent a few days beating my head against their API (and a few twitter DM convos with their PMs) to figure out the various ways I was holding things wrong.

I added a detailed comment, which reflects my observations (Good old, Dr. Empirical).  Hopefully it will keep others from falling into the same traps I did.
mattmoor: @gmlewis Anything I can do to help move this along? 😇 

gmlewis: You can always ping @wesleimp - he is one of the most responsive volunteer reviewers we have.  :smile:

Patch:
diff --git a/github/github-accessors.go b/github/github-accessors.go
index c2a158f1..3e2e0340 100644
--- a/github/github-accessors.go
+++ b/github/github-accessors.go
@@ -2972,6 +2972,14 @@ func (d *DraftReviewComment) GetBody() string {
 	return *d.Body
 }
 
+// GetLine returns the Line field if it's non-nil, zero value otherwise.
+func (d *DraftReviewComment) GetLine() int {
+	if d == nil || d.Line == nil {
+		return 0
+	}
+	return *d.Line
+}
+
 // GetPath returns the Path field if it's non-nil, zero value otherwise.
 func (d *DraftReviewComment) GetPath() string {
 	if d == nil || d.Path == nil {
@@ -2988,6 +2996,30 @@ func (d *DraftReviewComment) GetPosition() int {
 	return *d.Position
 }
 
+// GetSide returns the Side field if it's non-nil, zero value otherwise.
+func (d *DraftReviewComment) GetSide() string {
+	if d == nil || d.Side == nil {
+		return ""
+	}
+	return *d.Side
+}
+
+// GetStartLine returns the StartLine field if it's non-nil, zero value otherwise.
+func (d *DraftReviewComment) GetStartLine() int {
+	if d == nil || d.StartLine == nil {
+		return 0
+	}
+	return *d.StartLine
+}
+
+// GetStartSide returns the StartSide field if it's non-nil, zero value otherwise.
+func (d *DraftReviewComment) GetStartSide() string {
+	if d == nil || d.StartSide == nil {
+		return ""
+	}
+	return *d.StartSide
+}
+
 // GetAvatarURL returns the AvatarURL field if it's non-nil, zero value otherwise.
 func (e *Enterprise) GetAvatarURL() string {
 	if e == nil || e.AvatarURL == nil {
diff --git a/github/github.go b/github/github.go
index d876e98b..c68a80c7 100644
--- a/github/github.go
+++ b/github/github.go
@@ -231,7 +231,7 @@ type RawOptions struct {
 	Type RawType
 }
 
-// addOptions adds the parameters in opt as URL query parameters to s. opt
+// addOptions adds the parameters in opts as URL query parameters to s. opts
 // must be a struct whose fields may contain "url" tags.
 func addOptions(s string, opts interface{}) (string, error) {
 	v := reflect.ValueOf(opts)
diff --git a/github/pulls_reviews.go b/github/pulls_reviews.go
index 5bceb61c..42af0a0b 100644
--- a/github/pulls_reviews.go
+++ b/github/pulls_reviews.go
@@ -7,10 +7,13 @@ package github
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 )
 
+var ErrMixedCommentStyles = errors.New("cannot use both position and side/line form comments")
+
 // PullRequestReview represents a review of a pull request.
 type PullRequestReview struct {
 	ID             *int64     `json:"id,omitempty"`
@@ -36,6 +39,12 @@ type DraftReviewComment struct {
 	Path     *string `json:"path,omitempty"`
 	Position *int    `json:"position,omitempty"`
 	Body     *string `json:"body,omitempty"`
+
+	// The new comfort-fade-preview fields
+	StartSide *string `json:"start_side,omitempty"`
+	Side      *string `json:"side,omitempty"`
+	StartLine *int    `json:"start_line,omitempty"`
+	Line      *int    `json:"line,omitempty"`
 }
 
 func (c DraftReviewComment) String() string {
@@ -55,6 +64,32 @@ func (r PullRequestReviewRequest) String() string {
 	return Stringify(r)
 }
 
+func (r PullRequestReviewRequest) isComfortFadePreview() (bool, error) {
+	var isCF *bool
+	for _, comment := range r.Comments {
+		if comment == nil {
+			continue
+		}
+		hasPos := comment.Position != nil
+		hasComfortFade := (comment.StartSide != nil) || (comment.Side != nil) ||
+			(comment.StartLine != nil) || (comment.Line != nil)
+
+		switch {
+		case hasPos && hasComfortFade:
+			return false, ErrMixedCommentStyles
+		case hasPos && isCF != nil && *isCF:
+			return false, ErrMixedCommentStyles
+		case hasComfortFade && isCF != nil && !*isCF:
+			return false, ErrMixedCommentStyles
+		}
+		isCF = &hasComfortFade
+	}
+	if isCF != nil {
+		return *isCF, nil
+	}
+	return false, nil
+}
+
 // PullRequestReviewDismissalRequest represents a request to dismiss a review.
 type PullRequestReviewDismissalRequest struct {
 	Message *string `json:"message,omitempty"`
@@ -175,6 +210,38 @@ func (s *PullRequestsService) ListReviewComments(ctx context.Context, owner, rep
 // Read more about it here - https://github.com/google/go-github/issues/540
 //
 // GitHub API docs: https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review
+//
+// In order to use multi-line comments, you must use the "comfort fade" preview.
+// This replaces the use of the "Position" field in comments with 4 new fields:
+//   [Start]Side, and [Start]Line.
+// These new fields must be used for ALL comments (including single-line),
+// with the following restrictions (empirically observed, so subject to change).
+//
+// For single-line "comfort fade" comments, you must use:
+//
+//    Path:  &path,  // as before
+//    Body:  &body,  // as before
+//    Side:  &"RIGHT" (or "LEFT")
+//    Line:  &123,  // NOT THE SAME AS POSITION, this is an actual line number.
+//
+// If StartSide or StartLine is used with single-line comments, a 422 is returned.
+//
+// For multi-line "comfort fade" comments, you must use:
+//
+//    Path:      &path,  // as before
+//    Body:      &body,  // as before
+//    StartSide: &"RIGHT" (or "LEFT")
+//    Side:      &"RIGHT" (or "LEFT")
+//    StartLine: &120,
+//    Line:      &125,
+//
+// Suggested edits are made by commenting on the lines to replace, and including the
+// suggested edit in a block like this (it may be surrounded in non-suggestion markdown):
+//
+//    ```suggestion
+//    Use this instead.
+//    It is waaaaaay better.
+//    ```
 func (s *PullRequestsService) CreateReview(ctx context.Context, owner, repo string, number int, review *PullRequestReviewRequest) (*PullRequestReview, *Response, error) {
 	u := fmt.Sprintf("repos/%v/%v/pulls/%d/reviews", owner, repo, number)
 
@@ -183,6 +250,15 @@ func (s *PullRequestsService) CreateReview(ctx context.Context, owner, repo stri
 		return nil, nil, err
 	}
 
+	// Detect which style of review comment is being used.
+	if isCF, err := review.isComfortFadePreview(); err != nil {
+		return nil, nil, err
+	} else if isCF {
+		// If the review comments are using the comfort fade preview fields,
+		// then pass the comfort fade header.
+		req.Header.Set("Accept", mediaTypeMultiLineCommentsPreview)
+	}
+
 	r := new(PullRequestReview)
 	resp, err := s.client.Do(ctx, req, r)
 	if err != nil {


Test patch:
diff --git a/github/github-stringify_test.go b/github/github-stringify_test.go
index 96d2baf2..0d64461b 100644
--- a/github/github-stringify_test.go
+++ b/github/github-stringify_test.go
@@ -297,11 +297,15 @@ func TestDiscussionComment_String(t *testing.T) {
 
 func TestDraftReviewComment_String(t *testing.T) {
 	v := DraftReviewComment{
-		Path:     String(""),
-		Position: Int(0),
-		Body:     String(""),
+		Path:      String(""),
+		Position:  Int(0),
+		Body:      String(""),
+		StartSide: String(""),
+		Side:      String(""),
+		StartLine: Int(0),
+		Line:      Int(0),
 	}
-	want := `github.DraftReviewComment{Path:"", Position:0, Body:""}`
+	want := `github.DraftReviewComment{Path:"", Position:0, Body:"", StartSide:"", Side:"", StartLine:0, Line:0}`
 	if got := v.String(); got != want {
 		t.Errorf("DraftReviewComment.String = %v, want %v", got, want)
 	}
diff --git a/github/pulls_reviews_test.go b/github/pulls_reviews_test.go
index 25aaca1c..ae3fb7c5 100644
--- a/github/pulls_reviews_test.go
+++ b/github/pulls_reviews_test.go
@@ -73,7 +73,7 @@ func TestReviewers_marshall(t *testing.T) {
 				"created_at": ` + referenceTimeStr + `,
 				"url": "u"
 			}
-		], 
+		],
 		"teams" : [
 			{
 				"id": 1,
@@ -228,6 +228,121 @@ func TestPullRequestsService_ListReviewComments_withOptions(t *testing.T) {
 	}
 }
 
+func TestPullRequestReviewRequest_isComfortFadePreview(t *testing.T) {
+	path := "path/to/file.go"
+	body := "this is a comment body"
+	left, right := "LEFT", "RIGHT"
+	pos1, pos2, pos3 := 1, 2, 3
+	line1, line2, line3 := 11, 22, 33
+
+	tests := []struct {
+		name     string
+		review   *PullRequestReviewRequest
+		wantErr  error
+		wantBool bool
+	}{{
+		name:     "empty review",
+		review:   &PullRequestReviewRequest{},
+		wantBool: false,
+	}, {
+		name: "old-style review",
+		review: &PullRequestReviewRequest{
+			Comments: []*DraftReviewComment{{
+				Path:     &path,
+				Body:     &body,
+				Position: &pos1,
+			}, {
+				Path:     &path,
+				Body:     &body,
+				Position: &pos2,
+			}, {
+				Path:     &path,
+				Body:     &body,
+				Position: &pos3,
+			}},
+		},
+		wantBool: false,
+	}, {
+		name: "new-style review",
+		review: &PullRequestReviewRequest{
+			Comments: []*DraftReviewComment{{
+				Path: &path,
+				Body: &body,
+				Side: &right,
+				Line: &line1,
+			}, {
+				Path: &path,
+				Body: &body,
+				Side: &left,
+				Line: &line2,
+			}, {
+				Path: &path,
+				Body: &body,
+				Side: &right,
+				Line: &line3,
+			}},
+		},
+		wantBool: true,
+	}, {
+		name: "blended comment",
+		review: &PullRequestReviewRequest{
+			Comments: []*DraftReviewComment{{
+				Path:     &path,
+				Body:     &body,
+				Position: &pos1, // can't have both styles.
+				Side:     &right,
+				Line:     &line1,
+			}},
+		},
+		wantErr: ErrMixedCommentStyles,
+	}, {
+		name: "position then line",
+		review: &PullRequestReviewRequest{
+			Comments: []*DraftReviewComment{{
+				Path:     &path,
+				Body:     &body,
+				Position: &pos1,
+			}, {
+				Path: &path,
+				Body: &body,
+				Side: &right,
+				Line: &line1,
+			}},
+		},
+		wantErr: ErrMixedCommentStyles,
+	}, {
+		name: "line then position",
+		review: &PullRequestReviewRequest{
+			Comments: []*DraftReviewComment{{
+				Path: &path,
+				Body: &body,
+				Side: &right,
+				Line: &line1,
+			}, {
+				Path:     &path,
+				Body:     &body,
+				Position: &pos1,
+			}},
+		},
+		wantErr: ErrMixedCommentStyles,
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			gotBool, gotErr := tc.review.isComfortFadePreview()
+			if tc.wantErr != nil {
+				if gotErr != tc.wantErr {
+					t.Errorf("isComfortFadePreview() = %v, wanted %v", gotErr, tc.wantErr)
+				}
+			} else {
+				if gotBool != tc.wantBool {
+					t.Errorf("isComfortFadePreview() = %v, wanted %v", gotBool, tc.wantBool)
+				}
+			}
+		})
+	}
+}
+
 func TestPullRequestsService_ListReviewComments_invalidOwner(t *testing.T) {
 	client, _, _, teardown := setup()
 	defer teardown()