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

all: replace change.ReviewState with state.Review

The motivation to do so is to simplify state and make it easier to
develop related projects. There will be fewer conversions needed.
dmitshur committed 5 months ago commit 59022927fa7dea491c7a54393aec92ee4896717c
fs/fs.go
@@ -8,10 +8,11 @@ import (
 	"fmt"
 	"os"
 	"time"
 
 	"dmitri.shuralyov.com/service/change"
+	"dmitri.shuralyov.com/state"
 	"github.com/shurcooL/reactions"
 	"github.com/shurcooL/users"
 )
 
 type Service struct {
@@ -76,11 +77,11 @@ For convenience, a ` + "`" + `godoc` + "`" + ` view of this change can be seen [
 				},
 				change.Review{
 					ID:        "1",
 					User:      dmitshur,
 					CreatedAt: time.Date(2018, 2, 20, 21, 49, 35, 536092503, time.UTC),
-					State:     change.Approved,
+					State:     state.ReviewPlus2,
 					Body:      "There have been some out-of-band review comments that I've addressed. This will do for the initial version.\n\nLGTM.",
 				},
 				change.TimelineItem{
 					Actor:     dmitshur,
 					CreatedAt: time.Date(2018, 2, 20, 21, 57, 47, 537746502, time.UTC),
@@ -194,11 +195,11 @@ confidence in the approach, they can be factored out and made public.`,
 				},
 				change.Review{
 					ID:        "1",
 					User:      hajimehoshi,
 					CreatedAt: time.Date(2018, 10, 23, 3, 22, 57, 249312000, time.UTC),
-					State:     change.Approved,
+					State:     state.ReviewPlus2,
 					Body:      "I did a rough review and could not find a critical issue. 🙂",
 				},
 				change.TimelineItem{
 					Actor:     dmitshur,
 					CreatedAt: time.Date(2018, 10, 23, 3, 32, 2, 951463000, time.UTC),
gerritapi/gerritapi.go
@@ -10,10 +10,11 @@ import (
 	"strconv"
 	"strings"
 	"unicode"
 
 	"dmitri.shuralyov.com/service/change"
+	"dmitri.shuralyov.com/state"
 	"github.com/andygrunwald/go-gerrit"
 	"github.com/shurcooL/users"
 )
 
 // NewService creates a Gerrit-backed issues.Service using given Gerrit client.
@@ -62,11 +63,11 @@ func (s service) List(ctx context.Context, repo string, opt change.ListOptions)
 		if chg.Status == "DRAFT" {
 			continue
 		}
 		is = append(is, change.Change{
 			ID:        uint64(chg.Number),
-			State:     state(chg.Status),
+			State:     changeState(chg.Status),
 			Title:     chg.Subject,
 			Author:    s.gerritUser(chg.Owner),
 			CreatedAt: chg.Created.Time,
 			Replies:   len(chg.Messages),
 		})
@@ -97,21 +98,21 @@ func (s service) Get(ctx context.Context, repo string, id uint64) (change.Change
 	if chg.Status == "DRAFT" {
 		return change.Change{}, os.ErrNotExist
 	}
 	return change.Change{
 		ID:           id,
-		State:        state(chg.Status),
+		State:        changeState(chg.Status),
 		Title:        chg.Subject,
 		Author:       s.gerritUser(chg.Owner),
 		CreatedAt:    chg.Created.Time,
 		Replies:      len(chg.Messages),
 		Commits:      len(chg.Revisions),
 		ChangedFiles: 0, // TODO.
 	}, nil
 }
 
-func state(status string) change.State {
+func changeState(status string) change.State {
 	switch status {
 	case "NEW":
 		return change.OpenState
 	case "ABANDONED":
 		return change.ClosedState
@@ -318,11 +319,10 @@ func (s service) ListTimeline(ctx context.Context, repo string, id uint64, opt *
 				}
 				timeline = append(timeline, change.Review{
 					ID:        fmt.Sprint(idx), // TODO: message.ID is not uint64; e.g., "bfba753d015916303152305cee7152ea7a112fe0".
 					User:      s.gerritUser(message.Author),
 					CreatedAt: message.Date.Time,
-					State:     change.Commented,
 					Body:      body,
 					Editable:  false,
 					Comments:  cs,
 				})
 			case "gerrit:abandon":
@@ -366,11 +366,11 @@ func (s service) ListTimeline(ctx context.Context, repo string, id uint64, opt *
 					})
 				}
 			}
 		}
 		reviewState := reviewState(labels)
-		if body == "" && len(cs) == 0 && reviewState == change.Commented && labels != "" {
+		if body == "" && len(cs) == 0 && reviewState == state.ReviewNoScore && labels != "" {
 			// Skip an empty comment that, e.g., just sets a Run-TryBot+1 label.
 			continue
 		}
 		timeline = append(timeline, change.Review{
 			ID:        fmt.Sprint(idx), // TODO: message.ID is not uint64; e.g., "bfba753d015916303152305cee7152ea7a112fe0".
@@ -498,20 +498,24 @@ func matchNumber(s string, number int) int {
 		return -1
 	}
 	return len(a)
 }
 
-func reviewState(labels string) change.ReviewState {
+func reviewState(labels string) state.Review {
 	for _, label := range strings.Split(labels, " ") {
 		switch label {
 		case "Code-Review+2":
-			return change.Approved
+			return state.ReviewPlus2
+		case "Code-Review+1":
+			return state.ReviewPlus1
+		case "Code-Review-1":
+			return state.ReviewMinus1
 		case "Code-Review-2":
-			return change.ChangesRequested
+			return state.ReviewMinus2
 		}
 	}
-	return change.Commented
+	return state.ReviewNoScore
 }
 
 func (service) EditComment(_ context.Context, repo string, id uint64, cr change.CommentRequest) (change.Comment, error) {
 	return change.Comment{}, fmt.Errorf("EditComment: not implemented")
 }
githubapi/githubapi.go
@@ -9,10 +9,11 @@ import (
 	"sort"
 	"strings"
 
 	"dmitri.shuralyov.com/route/github"
 	"dmitri.shuralyov.com/service/change"
+	"dmitri.shuralyov.com/state"
 	githubv3 "github.com/google/go-github/github"
 	"github.com/shurcooL/githubv4"
 	"github.com/shurcooL/issues"
 	"github.com/shurcooL/reactions"
 	"github.com/shurcooL/users"
@@ -873,24 +874,27 @@ func ghPRState(state githubv4.PullRequestState) change.State {
 	default:
 		panic("unreachable")
 	}
 }
 
-// ghPRReviewState converts a GitHub PullRequestReviewState to change.ReviewState, if it's supported.
-func ghPRReviewState(state githubv4.PullRequestReviewState) (_ change.ReviewState, ok bool) {
-	switch state {
+// ghPRReviewState converts a GitHub PullRequestReviewState to state.Review, if it's supported.
+//
+// TODO: Consider using author association or so to tell approved by someone with
+//       push rights (+2) apart from approved by someone without push rights (+1), etc.
+func ghPRReviewState(st githubv4.PullRequestReviewState) (_ state.Review, ok bool) {
+	switch st {
 	case githubv4.PullRequestReviewStateApproved:
-		return change.Approved, true
+		return state.ReviewPlus2, true
 	case githubv4.PullRequestReviewStateCommented:
-		return change.Commented, true
+		return state.ReviewNoScore, true
 	case githubv4.PullRequestReviewStateChangesRequested:
-		return change.ChangesRequested, true
+		return state.ReviewMinus2, true
 	case githubv4.PullRequestReviewStateDismissed:
 		// PullRequestReviewStateDismissed are reviews that have been retroactively dismissed.
 		// Display them as a regular comment review for now (we can't know the original state).
 		// THINK: Consider displaying these more distinctly.
-		return change.Commented, true
+		return state.ReviewNoScore, true
 	case githubv4.PullRequestReviewStatePending:
 		// PullRequestReviewStatePending are reviews that are pending (haven't been posted yet).
 		// TODO: Consider displaying pending review comments. Figure this out
 		//       when adding ability to leave reviews.
 		return 0, false
maintner/maintner.go
@@ -9,10 +9,11 @@ import (
 	"sort"
 	"strings"
 	"unicode"
 
 	"dmitri.shuralyov.com/service/change"
+	"dmitri.shuralyov.com/state"
 	"github.com/shurcooL/issues"
 	"github.com/shurcooL/users"
 	"github.com/sourcegraph/go-diff/diff"
 	"golang.org/x/build/maintner"
 )
@@ -38,15 +39,15 @@ func (s service) List(_ context.Context, repo string, opt change.ListOptions) ([
 	var is []change.Change
 	err := project.ForeachCLUnsorted(func(cl *maintner.GerritCL) error {
 		if cl.Private {
 			return nil
 		}
-		state := state(cl.Status)
+		changeState := changeState(cl.Status)
 		switch {
-		case opt.Filter == change.FilterOpen && state != change.OpenState:
+		case opt.Filter == change.FilterOpen && changeState != change.OpenState:
 			return nil
-		case opt.Filter == change.FilterClosedMerged && !(state == change.ClosedState || state == change.MergedState):
+		case opt.Filter == change.FilterClosedMerged && !(changeState == change.ClosedState || changeState == change.MergedState):
 			return nil
 		}
 		var labels []issues.Label
 		cl.Meta.Hashtags().Foreach(func(hashtag string) {
 			labels = append(labels, issues.Label{
@@ -54,11 +55,11 @@ func (s service) List(_ context.Context, repo string, opt change.ListOptions) ([
 				Color: issues.RGB{R: 0xed, G: 0xed, B: 0xed}, // A default light gray.
 			})
 		})
 		is = append(is, change.Change{
 			ID:        uint64(cl.Number),
-			State:     state,
+			State:     changeState,
 			Title:     firstParagraph(cl.Commit.Msg),
 			Labels:    labels,
 			Author:    gerritUser(cl.Commit.Author),
 			CreatedAt: cl.Created,
 			Replies:   len(cl.Messages),
@@ -84,15 +85,15 @@ func (s service) Count(_ context.Context, repo string, opt change.ListOptions) (
 	var count uint64
 	err := project.ForeachCLUnsorted(func(cl *maintner.GerritCL) error {
 		if cl.Private {
 			return nil
 		}
-		state := state(cl.Status)
+		changeState := changeState(cl.Status)
 		switch {
-		case opt.Filter == change.FilterOpen && state != change.OpenState:
+		case opt.Filter == change.FilterOpen && changeState != change.OpenState:
 			return nil
-		case opt.Filter == change.FilterClosedMerged && !(state == change.ClosedState || state == change.MergedState):
+		case opt.Filter == change.FilterClosedMerged && !(changeState == change.ClosedState || changeState == change.MergedState):
 			return nil
 		}
 		count++
 		return nil
 	})
@@ -117,11 +118,11 @@ func (s service) Get(_ context.Context, repo string, id uint64) (change.Change,
 			Color: issues.RGB{R: 0xed, G: 0xed, B: 0xed}, // A default light gray.
 		})
 	})
 	return change.Change{
 		ID:           uint64(cl.Number),
-		State:        state(cl.Status),
+		State:        changeState(cl.Status),
 		Title:        firstParagraph(cl.Commit.Msg),
 		Labels:       labels,
 		Author:       gerritUser(cl.Commit.Author),
 		CreatedAt:    cl.Created,
 		Replies:      len(cl.Messages),
@@ -147,39 +148,30 @@ func (s service) ListTimeline(_ context.Context, repo string, id uint64, opt *ch
 		User:      gerritUser(cl.Commit.Author),
 		CreatedAt: cl.Created,
 		Body:      "", // THINK: Include commit message or no?
 	})
 	for _, m := range cl.Messages {
-		label, body, ok := parseMessage(m.Message)
+		labels, body, ok := parseMessage(m.Message)
 		if !ok {
 			timeline = append(timeline, change.Comment{
 				User:      gerritUser(m.Author),
 				CreatedAt: m.Date,
 				Body:      m.Message,
 			})
 			continue
 		}
-		var state change.ReviewState
-		switch label {
-		default:
-			state = change.Commented
-		case "Code-Review+2":
-			state = change.Approved
-		case "Code-Review-2":
-			state = change.ChangesRequested
-		}
 		timeline = append(timeline, change.Review{
 			User:      gerritUser(m.Author),
 			CreatedAt: m.Date,
-			State:     state,
+			State:     reviewState(labels),
 			Body:      body,
 		})
 	}
 	return timeline, nil
 }
 
-func parseMessage(m string) (label string, body string, ok bool) {
+func parseMessage(m string) (labels string, body string, ok bool) {
 	// "Patch Set ".
 	if !strings.HasPrefix(m, "Patch Set ") {
 		return "", "", false
 	}
 	m = m[len("Patch Set "):]
@@ -197,33 +189,49 @@ func parseMessage(m string) (label string, body string, ok bool) {
 	}
 	m = m[1:]
 
 	switch i = strings.IndexByte(m, '\n'); i {
 	case -1:
-		label = m
+		labels = m
 	default:
-		label = m[:i]
+		labels = m[:i]
 		body = m[i+1:]
 	}
 
-	if label != "" {
+	if labels != "" {
 		// " ".
-		if len(label) < 1 || label[0] != ' ' {
+		if len(labels) < 1 || labels[0] != ' ' {
 			return "", "", false
 		}
-		label = label[1:]
+		labels = labels[1:]
 	}
 
 	if body != "" {
 		// "\n".
 		if len(body) < 1 || body[0] != '\n' {
 			return "", "", false
 		}
 		body = body[1:]
 	}
 
-	return label, body, true
+	return labels, body, true
+}
+
+func reviewState(labels string) state.Review {
+	for _, label := range strings.Split(labels, " ") {
+		switch label {
+		case "Code-Review+2":
+			return state.ReviewPlus2
+		case "Code-Review+1":
+			return state.ReviewPlus1
+		case "Code-Review-1":
+			return state.ReviewMinus1
+		case "Code-Review-2":
+			return state.ReviewMinus2
+		}
+	}
+	return state.ReviewNoScore
 }
 
 func (s service) ListCommits(_ context.Context, repo string, id uint64) ([]change.Commit, error) {
 	s.c.RLock()
 	defer s.c.RUnlock()
@@ -279,11 +287,11 @@ func (s service) GetDiff(_ context.Context, repo string, id uint64, opt *change.
 
 func (service) EditComment(_ context.Context, repo string, id uint64, cr change.CommentRequest) (change.Comment, error) {
 	return change.Comment{}, fmt.Errorf("EditComment: not implemented")
 }
 
-func state(status string) change.State {
+func changeState(status string) change.State {
 	switch status {
 	case "new":
 		return change.OpenState
 	case "abandoned":
 		return change.ClosedState
timeline.go
@@ -1,10 +1,11 @@
 package change
 
 import (
 	"time"
 
+	"dmitri.shuralyov.com/state"
 	"github.com/shurcooL/issues"
 	"github.com/shurcooL/reactions"
 	"github.com/shurcooL/users"
 )
 
@@ -30,11 +31,11 @@ type Edited struct {
 type Review struct {
 	ID        string
 	User      users.User
 	CreatedAt time.Time
 	Edited    *Edited // Edited is nil if the review hasn't been edited.
-	State     ReviewState
+	State     state.Review
 	Body      string // Optional.
 	Reactions []reactions.Reaction
 	Editable  bool // Editable represents whether the current user (if any) can perform edit operations on this review.
 	Comments  []InlineComment
 }
@@ -46,18 +47,10 @@ type InlineComment struct {
 	Line      int
 	Body      string
 	Reactions []reactions.Reaction
 }
 
-type ReviewState int8
-
-const (
-	Approved         ReviewState = +1
-	Commented        ReviewState = 0
-	ChangesRequested ReviewState = -1
-)
-
 // TimelineItem represents a timeline item.
 type TimelineItem struct {
 	ID        string // TODO: See if this belongs here.
 	Actor     users.User
 	CreatedAt time.Time