dmitri.shuralyov.com/service/change/...

Merge ListComments, ListEvents into ListTimeline.

There's no longer a clean distinction between comments and events.
There are other timeline item types that don't fit into either category
cleanly. Also, underlying implementations for various APIs are very
similar and have overlap. As a result, it makes sense to merge these
two methods into one.
dmitshur committed 2 years ago commit 10c5b0f59dc1ffb8cf59c4a22110e89f803affa5
changes.go
@@ -16,19 +16,17 @@ type Service interface {
 	// Count changes.
 	Count(ctx context.Context, repo string, opt ListOptions) (uint64, error)
 
 	// Get a change.
 	Get(ctx context.Context, repo string, id uint64) (Change, error)
+
+	// ListTimeline lists timeline items (issues.Comment, issues.Event) for specified change id.
+	ListTimeline(ctx context.Context, repo string, id uint64, opt *ListTimelineOptions) ([]interface{}, error)
 	// ListCommits lists change commits.
 	ListCommits(ctx context.Context, repo string, id uint64) ([]Commit, error)
 	// Get a change diff.
 	GetDiff(ctx context.Context, repo string, id uint64, opt *GetDiffOptions) ([]byte, error)
-
-	// ListComments lists comments for specified change id.
-	ListComments(ctx context.Context, repo string, id uint64, opt *ListCommentsOptions) ([]issues.Comment, error)
-	// ListEvents lists events for specified change id.
-	ListEvents(ctx context.Context, repo string, id uint64, opt *ListCommentsOptions) ([]issues.Event, error)
 }
 
 // Change represents a change in a repository.
 type Change struct {
 	ID        uint64
@@ -59,11 +57,11 @@ const (
 	ClosedState State = "closed"
 	// MergedState is when a change is merged.
 	MergedState State = "merged"
 )
 
-// ListOptions are options for list operations.
+// ListOptions are options for list and count operations.
 type ListOptions struct {
 	Filter StateFilter
 }
 
 // StateFilter is a filter by state.
@@ -76,18 +74,18 @@ const (
 	FilterClosedMerged StateFilter = "closed|merged"
 	// FilterAll is a state filter that includes all changes.
 	FilterAll StateFilter = "all"
 )
 
-type GetDiffOptions struct {
-	// Commit is the commit ID of the commit to fetch.
-	Commit string
-}
-
-// ListCommentsOptions controls pagination.
-type ListCommentsOptions struct {
+// ListTimelineOptions controls pagination.
+type ListTimelineOptions struct {
 	// Start is the index of first result to retrieve, zero-indexed.
 	Start int
 
 	// Length is the number of results to include.
 	Length int
 }
+
+type GetDiffOptions struct {
+	// Commit is the commit ID of the commit to fetch.
+	Commit string
+}
githubapi/githubapi.go
@@ -247,19 +247,21 @@ func (s service) GetDiff(ctx context.Context, rs string, id uint64, opt *changes
 		}
 		return []byte(diff), nil
 	}
 }
 
-func (s service) ListComments(ctx context.Context, rs string, id uint64, opt *changes.ListCommentsOptions) ([]issues.Comment, error) {
-	// TODO: Respect opt.Start and opt.Length, if given.
-
+func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *changes.ListTimelineOptions) ([]interface{}, error) {
 	repo, err := ghRepoSpec(rs)
 	if err != nil {
+		// TODO: Map to 400 Bad Request HTTP error.
 		return nil, err
 	}
-	var comments []issues.Comment
 
+	type event struct { // Common fields for all events.
+		Actor     githubqlActor
+		CreatedAt githubql.DateTime
+	}
 	var q struct {
 		Repository struct {
 			PullRequest struct {
 				Author          githubqlActor
 				PublishedAt     githubql.DateTime
@@ -267,159 +269,10 @@ func (s service) ListComments(ctx context.Context, rs string, id uint64, opt *ch
 				Editor          *githubqlActor
 				Body            githubql.String
 				ReactionGroups  reactionGroups
 				ViewerCanUpdate githubql.Boolean
 
-				// TODO: Combine with first page of Comments...
-			} `graphql:"pullRequest(number:$prNumber)"`
-		} `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"`
-	}
-	variables := map[string]interface{}{
-		"repositoryOwner": githubql.String(repo.Owner),
-		"repositoryName":  githubql.String(repo.Repo),
-		"prNumber":        githubql.Int(id),
-	}
-	err = s.clV4.Query(ctx, &q, variables)
-	if err != nil {
-		return comments, err
-	}
-	pr := q.Repository.PullRequest
-	reactions, err := s.reactions(pr.ReactionGroups)
-	if err != nil {
-		return comments, err
-	}
-	var edited *issues.Edited
-	if pr.LastEditedAt != nil {
-		edited = &issues.Edited{
-			By: ghActor(*pr.Editor),
-			At: pr.LastEditedAt.Time,
-		}
-	}
-	comments = append(comments, issues.Comment{
-		ID:        prDescriptionCommentID,
-		User:      ghActor(pr.Author),
-		CreatedAt: pr.PublishedAt.Time,
-		Edited:    edited,
-		Body:      string(pr.Body),
-		Reactions: reactions,
-		Editable:  bool(pr.ViewerCanUpdate),
-	})
-
-	{
-		var q struct {
-			Repository struct {
-				PullRequest struct {
-					Comments struct {
-						Nodes []struct {
-							DatabaseID      githubql.Int
-							Author          githubqlActor
-							PublishedAt     githubql.DateTime
-							LastEditedAt    *githubql.DateTime
-							Editor          *githubqlActor
-							Body            githubql.String
-							ReactionGroups  reactionGroups
-							ViewerCanUpdate githubql.Boolean
-						}
-						PageInfo struct {
-							EndCursor   githubql.String
-							HasNextPage githubql.Boolean
-						}
-					} `graphql:"comments(first:1,after:$commentsCursor)"` // TODO: Increase page size too 100 after done testing.
-					Reviews struct {
-						Nodes []struct {
-							DatabaseID      uint64
-							Author          githubqlActor
-							PublishedAt     githubql.DateTime
-							LastEditedAt    *githubql.DateTime
-							Editor          *githubqlActor
-							Body            string
-							ViewerCanUpdate bool
-						}
-						PageInfo struct {
-							EndCursor   githubql.String
-							HasNextPage githubql.Boolean
-						}
-					} `graphql:"reviews(first:100)"` // TODO: Figure out how to make pagination across 2 resource types work...
-				} `graphql:"pullRequest(number:$prNumber)"`
-			} `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"`
-		}
-		variables := map[string]interface{}{
-			"repositoryOwner": githubql.String(repo.Owner),
-			"repositoryName":  githubql.String(repo.Repo),
-			"prNumber":        githubql.Int(id),
-			"commentsCursor":  (*githubql.String)(nil),
-		}
-		for {
-			err := s.clV4.Query(ctx, &q, variables)
-			if err != nil {
-				return comments, err
-			}
-			for _, comment := range q.Repository.PullRequest.Comments.Nodes {
-				reactions, err := s.reactions(comment.ReactionGroups)
-				if err != nil {
-					return comments, err
-				}
-				var edited *issues.Edited
-				if comment.LastEditedAt != nil {
-					edited = &issues.Edited{
-						By: ghActor(*comment.Editor),
-						At: comment.LastEditedAt.Time,
-					}
-				}
-				comments = append(comments, issues.Comment{
-					ID:        uint64(comment.DatabaseID),
-					User:      ghActor(comment.Author),
-					CreatedAt: comment.PublishedAt.Time,
-					Edited:    edited,
-					Body:      string(comment.Body),
-					Reactions: reactions,
-					Editable:  bool(comment.ViewerCanUpdate),
-				})
-			}
-			if !q.Repository.PullRequest.Comments.PageInfo.HasNextPage {
-				break
-			}
-			variables["commentsCursor"] = githubql.NewString(q.Repository.PullRequest.Comments.PageInfo.EndCursor)
-		}
-		for _, review := range q.Repository.PullRequest.Reviews.Nodes {
-			if review.Body == "" {
-				continue
-			}
-			var edited *issues.Edited
-			if review.LastEditedAt != nil {
-				edited = &issues.Edited{
-					By: ghActor(*review.Editor),
-					At: review.LastEditedAt.Time,
-				}
-			}
-			comments = append(comments, issues.Comment{
-				ID:        review.DatabaseID,
-				User:      ghActor(review.Author),
-				CreatedAt: review.PublishedAt.Time,
-				Edited:    edited,
-				Body:      review.Body,
-				Editable:  review.ViewerCanUpdate,
-			})
-		}
-	}
-
-	return comments, nil
-}
-
-func (s service) ListEvents(ctx context.Context, rs string, id uint64, opt *changes.ListCommentsOptions) ([]issues.Event, error) {
-	repo, err := ghRepoSpec(rs)
-	if err != nil {
-		// TODO: Map to 400 Bad Request HTTP error.
-		return nil, err
-	}
-	type event struct { // Common fields for all events.
-		Actor     githubqlActor
-		CreatedAt githubql.DateTime
-	}
-	var q struct {
-		Repository struct {
-			PullRequest struct {
 				Timeline struct {
 					Nodes []struct {
 						Typename    string `graphql:"__typename"`
 						ClosedEvent struct {
 							event
@@ -466,11 +319,34 @@ func (s service) ListEvents(ctx context.Context, rs string, id uint64, opt *chan
 							Author    githubqlActor
 							CreatedAt githubql.DateTime
 							State     githubql.PullRequestReviewState
 						} `graphql:"...on PullRequestReview"`
 					}
-				} `graphql:"timeline(first:100)"` // TODO: Paginate?
+				} `graphql:"timeline(first:100)"` // TODO: Pagination...
+				Comments struct {
+					Nodes []struct {
+						DatabaseID      githubql.Int
+						Author          githubqlActor
+						PublishedAt     githubql.DateTime
+						LastEditedAt    *githubql.DateTime
+						Editor          *githubqlActor
+						Body            githubql.String
+						ReactionGroups  reactionGroups
+						ViewerCanUpdate githubql.Boolean
+					}
+				} `graphql:"comments(first:100)"` // TODO: Pagination...
+				Reviews struct {
+					Nodes []struct {
+						DatabaseID      uint64
+						Author          githubqlActor
+						PublishedAt     githubql.DateTime
+						LastEditedAt    *githubql.DateTime
+						Editor          *githubqlActor
+						Body            string
+						ViewerCanUpdate bool
+					}
+				} `graphql:"reviews(first:100)"` // TODO: Figure out how to make pagination across 2 resource types work...
 			} `graphql:"pullRequest(number:$prNumber)"`
 		} `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"`
 	}
 	variables := map[string]interface{}{
 		"repositoryOwner": githubql.String(repo.Owner),
@@ -479,11 +355,76 @@ func (s service) ListEvents(ctx context.Context, rs string, id uint64, opt *chan
 	}
 	err = s.clV4.Query(ctx, &q, variables)
 	if err != nil {
 		return nil, err
 	}
-	var events []issues.Event
+	var timeline []interface{}
+	{
+		pr := q.Repository.PullRequest
+		reactions, err := s.reactions(pr.ReactionGroups)
+		if err != nil {
+			return timeline, err
+		}
+		var edited *issues.Edited
+		if pr.LastEditedAt != nil {
+			edited = &issues.Edited{
+				By: ghActor(*pr.Editor),
+				At: pr.LastEditedAt.Time,
+			}
+		}
+		timeline = append(timeline, issues.Comment{
+			ID:        prDescriptionCommentID,
+			User:      ghActor(pr.Author),
+			CreatedAt: pr.PublishedAt.Time,
+			Edited:    edited,
+			Body:      string(pr.Body),
+			Reactions: reactions,
+			Editable:  bool(pr.ViewerCanUpdate),
+		})
+	}
+	for _, comment := range q.Repository.PullRequest.Comments.Nodes {
+		reactions, err := s.reactions(comment.ReactionGroups)
+		if err != nil {
+			return timeline, err
+		}
+		var edited *issues.Edited
+		if comment.LastEditedAt != nil {
+			edited = &issues.Edited{
+				By: ghActor(*comment.Editor),
+				At: comment.LastEditedAt.Time,
+			}
+		}
+		timeline = append(timeline, issues.Comment{
+			ID:        uint64(comment.DatabaseID),
+			User:      ghActor(comment.Author),
+			CreatedAt: comment.PublishedAt.Time,
+			Edited:    edited,
+			Body:      string(comment.Body),
+			Reactions: reactions,
+			Editable:  bool(comment.ViewerCanUpdate),
+		})
+	}
+	for _, review := range q.Repository.PullRequest.Reviews.Nodes {
+		if review.Body == "" {
+			continue
+		}
+		var edited *issues.Edited
+		if review.LastEditedAt != nil {
+			edited = &issues.Edited{
+				By: ghActor(*review.Editor),
+				At: review.LastEditedAt.Time,
+			}
+		}
+		timeline = append(timeline, issues.Comment{
+			ID:        review.DatabaseID,
+			User:      ghActor(review.Author),
+			CreatedAt: review.PublishedAt.Time,
+			Edited:    edited,
+			Body:      review.Body,
+			Editable:  review.ViewerCanUpdate,
+		})
+	}
 	for _, event := range q.Repository.PullRequest.Timeline.Nodes {
 		e := issues.Event{
 			//ID:   0, // TODO.
 			Type: ghEventType(event.Typename),
 		}
@@ -540,26 +481,28 @@ func (s service) ListEvents(ctx context.Context, rs string, id uint64, opt *chan
 			e.Actor = ghActor(event.PullRequestReview.Author)
 			e.CreatedAt = event.PullRequestReview.CreatedAt.Time
 		default:
 			continue
 		}
-		events = append(events, e)
+		timeline = append(timeline, e)
 	}
-	// We can't just delegate pagination to GitHub because our events don't match up 1:1,
+
+	// We can't just delegate pagination to GitHub because our timeline items don't match up 1:1,
 	// we want to skip IssueComment in the timeline, etc.
+	// THINK: Or maybe they can be made to match up? Investigate.
 	if opt != nil {
 		start := opt.Start
-		if start > len(events) {
-			start = len(events)
+		if start > len(timeline) {
+			start = len(timeline)
 		}
 		end := opt.Start + opt.Length
-		if end > len(events) {
-			end = len(events)
+		if end > len(timeline) {
+			end = len(timeline)
 		}
-		events = events[start:end]
+		timeline = timeline[start:end]
 	}
-	return events, nil
+	return timeline, nil
 }
 
 type repoSpec struct {
 	Owner string
 	Repo  string