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 years ago commit 59022927fa7dea491c7a54393aec92ee4896717c
Collapse all
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