feat(mutable-reviews): Comment ordering respects review tags (#110)

Part of #85

server/internal/github/api_source_prme.go

@@ -22,7 +22,15 @@ func (s *CommentsAPISource) GetReview(ctx context.Context) (api.Review, error) {
 		return api.Review{}, errors.WithStack(err)
 	}
 
-	model, err := FetchReviewModel(ctx, params, CommentMatchesTag(s.ReviewParamsMap.Tag))
+	model, err := FetchReviewModel(
+		ctx,
+		params,
+		CommentMatchesTag(s.ReviewParamsMap.Tag),
+		func(body string) (int, bool) {
+			t, ok := parseReviewTag(body)
+			return t.Order, ok
+		},
+	)
 	if err != nil {
 		return api.Review{}, errors.WithStack(err)
 	}

server/internal/github/api_source_review.go

@@ -25,7 +25,7 @@ func (s *ReviewAPISource) GetReview(ctx context.Context) (api.Review, error) {
 		return api.Review{}, errors.New("review must have review id")
 	}
 
-	model, err := FetchReviewModel(ctx, params, CommentMatchesReview(params.ReviewID))
+	model, err := FetchReviewModel(ctx, params, CommentMatchesReview(params.ReviewID), orderOf)
 	if err != nil {
 		return api.Review{}, errors.WithStack(err)
 	}

server/internal/github/comment.go

@@ -1,11 +1,25 @@
 package github
 
-import "github.com/stanistan/present-me/internal/api"
+import (
+	"regexp"
+	"strconv"
+	"strings"
+
+	"github.com/stanistan/present-me/internal/api"
+	"github.com/stanistan/present-me/internal/errors"
+	"github.com/stanistan/present-me/internal/github/diff"
+)
 
 func transformComments(cs []*PullRequestComment) []api.Comment {
 	out := make([]api.Comment, len(cs))
 	for idx, c := range cs {
 		c := c
+
+		diff, err := commentCodeDiff(c)
+		if err != nil {
+			diff = *c.DiffHunk
+		}
+
 		out[idx] = api.Comment{
 			Number: idx + 1,
 			Title: api.MaybeLinked{
@@ -15,7 +29,7 @@ func transformComments(cs []*PullRequestComment) []api.Comment {
 			Description: commentBody(*c.Body),
 			CodeBlock: api.CodeBlock{
 				IsDiff:   true,
-				Content:  *c.DiffHunk,
+				Content:  diff,
 				Language: detectLanguage(*c.Path),
 			},
 		}
@@ -27,3 +41,46 @@ func commentBody(s string) string {
 	out := startsWithNumberRegexp.ReplaceAllString(s, "")
 	return prmeTagRegexp.ReplaceAllString(out, "")
 }
+
+func commentCodeDiff(c *PullRequestComment) (string, error) {
+	// we extract the metadata, we know which side we are going to be starting on.
+	meta, err := diff.ParseHunkMeta(*c.DiffHunk)
+	if err != nil {
+		return "", errors.WithStack(err)
+	}
+
+	// how are we counting lines?
+	hunkRange, err := meta.RangeForSide(*c.Side)
+	if err != nil {
+		return "", errors.WithStack(err)
+	}
+
+	// - endLine is the line that the comment is on or after,
+	// - startLine is the beginning line that we'll include in our diff,
+	//   and it looks like github defaults to 4 lines included if there is
+	//   no `StartLine`.
+	scanner, err := diff.NewScanner(
+		hunkRange,
+		diff.RangeFrom{c.OriginalStartLine, c.StartLine},
+		diff.RangeFrom{c.OriginalLine, c.Line},
+	)
+	if err != nil {
+		return "", errors.WithStack(err)
+	}
+
+	// filter our diff lines to only what's relevant for this comment
+	out := scanner.Filter(strings.Split(*c.DiffHunk, "\n"))
+	return strings.Join(out, "\n"), nil
+}
+
+var startsWithNumberRegexp = regexp.MustCompile(`^\s*(\d+)\.\s*`)
+
+func orderOf(c string) (int, bool) {
+	m := startsWithNumberRegexp.FindStringSubmatch(c)
+	if m == nil {
+		return 0, false
+	}
+
+	n, _ := strconv.Atoi(m[1])
+	return n, true
+}

server/internal/github/diff_test.go

@@ -32,7 +32,7 @@ func TestDiffGenerator(t *testing.T) {
 			assert.NoError(t, err, "test data file is an invalid json PullRequestComment")
 			comment.DiffHunk = &input
 
-			diff, err := generateDiff(&comment)
+			diff, err := commentCodeDiff(&comment)
 			assert.NoError(t, err, "failed to generate diff")
 			assert.Equal(t, expected, diff+"\n", "generated diff doesn't match expected output")
 		})

server/internal/github/github.go

@@ -32,7 +32,7 @@ type PrivateKey struct {
 	File string `name:"file" env:"GH_PK_FILE"`
 }
 
-func (o *ClientOptions) HTTPClient(ctx context.Context) (*http.Client, error) {
+func (o *ClientOptions) httpClient(ctx context.Context) (*http.Client, error) {
 	var (
 		itr http.RoundTripper
 		err error
@@ -59,7 +59,7 @@ type Client struct {
 }
 
 func New(ctx context.Context, opts ClientOptions) (*Client, error) {
-	c, err := opts.HTTPClient(ctx)
+	c, err := opts.httpClient(ctx)
 	if err != nil {
 		return nil, err
 	}
@@ -167,13 +167,18 @@ func (g *Client) ListComments(
 	)
 }
 
-func FetchReviewModel(ctx context.Context, r *ReviewParams, pred CommentPredicate) (*ReviewModel, error) {
+func FetchReviewModel(
+	ctx context.Context,
+	r *ReviewParams,
+	pred CommentPredicate,
+	orderOf func(string) (int, bool),
+) (*ReviewModel, error) {
 	client, ok := Ctx(ctx)
 	if !ok || client == nil {
 		return nil, errors.New("context missing github client")
 	}
 
-	model, err := client.FetchReviewModel(ctx, r, pred)
+	model, err := client.FetchReviewModel(ctx, r, pred, orderOf)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
@@ -182,7 +187,10 @@ func FetchReviewModel(ctx context.Context, r *ReviewParams, pred CommentPredicat
 }
 
 func (g *Client) FetchReviewModel(
-	ctx context.Context, r *ReviewParams, pred CommentPredicate,
+	ctx context.Context,
+	r *ReviewParams,
+	pred CommentPredicate,
+	orderOf func(string) (int, bool),
 ) (*ReviewModel, error) {
 	model := &ReviewModel{Params: r}
 	group, ctx := errgroup.WithContext(ctx)
@@ -195,6 +203,7 @@ func (g *Client) FetchReviewModel(
 		return err
 	})
 
+	// N.B. this is legacy-ish
 	if r.ReviewID != 0 {
 		group.Go(func() error {
 			review, err := g.GetReview(ctx, r)
@@ -208,20 +217,6 @@ func (g *Client) FetchReviewModel(
 	group.Go(func() error {
 		comments, err := g.ListComments(ctx, r, pred)
 		if err == nil {
-			for idx := range comments {
-				comment := comments[idx]
-				diff, err := generateDiff(comment)
-				if err != nil {
-					// TODO(stanistan):
-					// consider logging the warning here, and not mutating the diff as well,
-					// or _also_ returning the fact that there's an error warning in the API
-					// response.
-					return err
-				}
-
-				comment.DiffHunk = &diff
-				comments[idx] = comment
-			}
 			model.Comments = comments
 		}
 		return err

server/internal/github/predicate.go

@@ -15,21 +15,21 @@ func CommentMatchesReview(reviewID int64) CommentPredicate {
 	}
 }
 
-var prmeTagRegexp = regexp.MustCompile(`\(prme([^-\s]+)?(-(\d+))?\)\s*$`)
-
-type reviewTag struct {
+type ReviewTag struct {
 	Review string
 	Order  int
 }
 
-func parseReviewTag(s string) (reviewTag, bool) {
+var prmeTagRegexp = regexp.MustCompile(`\(prme([^-\s]+)?(-(\d+))?\)\s*$`)
+
+func parseReviewTag(s string) (ReviewTag, bool) {
 	m := prmeTagRegexp.FindStringSubmatch(s)
 	if m == nil {
-		return reviewTag{}, false
+		return ReviewTag{}, false
 	}
 
 	n, _ := strconv.Atoi(m[3])
-	return reviewTag{Review: m[1], Order: n}, true
+	return ReviewTag{Review: m[1], Order: n}, true
 }
 
 func CommentMatchesTag(tag string) CommentPredicate {

server/internal/github/predicate_test.go

@@ -11,36 +11,36 @@ func TestCommentTagRegex(t *testing.T) {
 		name       string
 		input      string
 		expectedOk bool
-		expected   reviewTag
+		expected   ReviewTag
 	}
 
 	for _, data := range []testData{
 		{
-			"standard", "foo (prme1)", true, reviewTag{"1", 0},
+			"standard", "foo (prme1)", true, ReviewTag{"1", 0},
 		},
 		{
-			"no-prefix", "(prme2)", true, reviewTag{"2", 0},
+			"no-prefix", "(prme2)", true, ReviewTag{"2", 0},
 		},
 		{
-			"trailing space", "(prme2)  ", true, reviewTag{"2", 0},
+			"trailing space", "(prme2)  ", true, ReviewTag{"2", 0},
 		},
 		{
-			"with order", "end of text (prmesomething-1)\n", true, reviewTag{"something", 1},
+			"with order", "end of text (prmesomething-1)\n", true, ReviewTag{"something", 1},
 		},
 		{
-			"with order 3", "(prmesomething-3)", true, reviewTag{"something", 3},
+			"with order 3", "(prmesomething-3)", true, ReviewTag{"something", 3},
 		},
 		{
-			"doesnt match in the middle", "(prme1) banana", false, reviewTag{},
+			"doesnt match in the middle", "(prme1) banana", false, ReviewTag{},
 		},
 		{
-			"can have naked prme", "foo (prme)", true, reviewTag{"", 0},
+			"can have naked prme", "foo (prme)", true, ReviewTag{"", 0},
 		},
 		{
-			"won't parse one without parentheses", "foo prme", false, reviewTag{},
+			"won't parse one without parentheses", "foo prme", false, ReviewTag{},
 		},
 		{
-			"no tag but order", "anything (prme-5)", true, reviewTag{"", 5},
+			"no tag but order", "anything (prme-5)", true, ReviewTag{"", 5},
 		},
 	} {
 		d := data

server/internal/github/review_model.go

@@ -1,14 +1,5 @@
 package github
 
-import (
-	"regexp"
-	"strconv"
-	"strings"
-
-	"github.com/stanistan/present-me/internal/errors"
-	"github.com/stanistan/present-me/internal/github/diff"
-)
-
 type ReviewModel struct {
 	Params *ReviewParams `json:"params"`
 
@@ -22,46 +13,3 @@ type ReviewFile struct {
 	IsAnnotated bool        `json:"isAnnotated"`
 	File        *CommitFile `json:"file"`
 }
-
-var startsWithNumberRegexp = regexp.MustCompile(`^\s*(\d+)\.\s*`)
-
-func orderOf(c string) (int, bool) {
-	m := startsWithNumberRegexp.FindStringSubmatch(c)
-	if m == nil {
-		return 0, false
-	}
-
-	n, _ := strconv.Atoi(m[1])
-	return n, true
-}
-
-func generateDiff(c *PullRequestComment) (string, error) {
-	// we extract the metadata, we know which side we are going to be starting on.
-	meta, err := diff.ParseHunkMeta(*c.DiffHunk)
-	if err != nil {
-		return "", errors.WithStack(err)
-	}
-
-	// how are we counting lines?
-	hunkRange, err := meta.RangeForSide(*c.Side)
-	if err != nil {
-		return "", errors.WithStack(err)
-	}
-
-	// - endLine is the line that the comment is on or after,
-	// - startLine is the beginning line that we'll include in our diff,
-	//   and it looks like github defaults to 4 lines included if there is
-	//   no `StartLine`.
-	scanner, err := diff.NewScanner(
-		hunkRange,
-		diff.RangeFrom{c.OriginalStartLine, c.StartLine},
-		diff.RangeFrom{c.OriginalLine, c.Line},
-	)
-	if err != nil {
-		return "", errors.WithStack(err)
-	}
-
-	// filter our diff lines to only what's relevant for this comment
-	out := scanner.Filter(strings.Split(*c.DiffHunk, "\n"))
-	return strings.Join(out, "\n"), nil
-}