Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add coloring for review state in "git appraise list" #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add coloring for review state in "git appraise list" #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92900a3
Add coloring for review state in "git appraise list"
llorens Jan 10, 2019
da84096
Follow code review comments.
llorens Jan 16, 2019
fa20e5e
More naming problems.
llorens Jan 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion commands/output/output.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"
"time"

"github.com/google/git-appraise/repository"
"github.com/google/git-appraise/review"
)

Expand Down Expand Up @@ -52,6 +53,15 @@ status: %s
// Number of lines of context to print for inline comments
contextLineCount = 5
)
var defaultColors = map[string]string{
"tbr": "red white bold blink",
"pending": "cyan",
"submitted": "yellow",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the colors for "pending" and "submitted" should be swapped.

I think yellow matches better with "pending" because it indicates that attention is needed. Conversely, cyan makes me think more along the lines of "no action necessary", which would match "submitted".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I associate yellow with something old like the paper of an old book becomes yellowish.
There's magenta – which I find pretty aggressive – used for "abandon". How about:

	"pending": "magenta",
	"submitted": "cyan",
	"abandon": "yellow",

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That mapping sounds good to me.

"accepted": "green",
"danger": "yellow red bold blink",
"abandon": "magenta",
"rejected": "yellow red bold strike",
}

// getStatusString returns a human friendly string encapsulating both the review's
// resolved status, and its submitted status.
Expand All @@ -77,11 +87,44 @@ func getStatusString(r *review.Summary) string {
return "rejected"
}

func getColorStatusString(repo repository.Repo, statusString string) string {
useColor, err := repo.GetColorBool("color.appraise")
if err != nil || !useColor {
return statusString
}

var colorOn string
var colorOff string

defaultColor, _ := defaultColors[statusString]
colorOn, err = repo.GetColor(
fmt.Sprintf("color.appraise.%s", statusString),
defaultColor,
)
if err != nil {
return statusString
}

colorOff, err = repo.GetColor("", "reset")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to reduce the amount of nesting for code blocks by short-circuiting when possible.

In this case, we can check if !useColor right here, and then return fmt.Printf(reviewSummaryTemplate, statusString, r.Revision, indentedDescription).

i.e.

	if !useColor {
		fmt.Printf(reviewSummaryTemplate, statusString, r.Revision, indentedDescription)
		return
	}

if err != nil {
return statusString
}

return fmt.Sprintf("%s%s%s", colorOn, statusString, colorOff)
}

// PrintSummary prints a single-line summary of a review.
func PrintSummary(r *review.Summary) {
statusString := getStatusString(r)
indentedDescription := strings.Replace(r.Request.Description, "\n", "\n ", -1)
fmt.Printf(reviewSummaryTemplate, statusString, r.Revision, indentedDescription)

fmt.Printf(
reviewSummaryTemplate,
getColorStatusString(r.Repo, statusString),
r.Revision,
indentedDescription,
)
}

// reformatTimestamp takes a timestamp string of the form "0123456789" and changes it
Expand Down
9 changes: 9 additions & 0 deletions repository/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,3 +985,12 @@ func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
}
return updatedReviews, nil
}

func (repo *GitRepo) GetColorBool(name string) (bool, error) {
err := repo.runGitCommandInline("config", "--get-colorbool", name)
return (err == nil), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense for this function to return (bool, error) as opposed to bool only, but I understand from your comment that this is a convention so I oblige.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you mean, and with the current implementation it does not make sense.

However, we actually can distinguish between the error produced by having an exit code of 1 vs. other types of errors.

I'm not asking for you to add the code to make that distinction in this PR, as it is a bit complicated to do, but it is helpful to make the API consistent with that if we want to add that logic later on.

}

func (repo *GitRepo) GetColor(name, defaultValue string) (string, error) {
return repo.runGitCommand("config", "--get-color", name, defaultValue)
}
8 changes: 8 additions & 0 deletions repository/mock_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,3 +611,11 @@ func (repo *mockRepoForTest) FetchAndReturnNewReviewHashes(remote, notesRefPatte
archiveRefPattern string) ([]string, error) {
return nil, nil
}

func (repo *mockRepoForTest) GetColorBool(name string) (bool, error) {
return false, nil
}

func (repo *mockRepoForTest) GetColor(name, defaultValue string) (string, error) {
return "", nil
}
6 changes: 6 additions & 0 deletions repository/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ type Repo interface {
// GetSubmitStrategy returns the way in which a review is submitted
GetSubmitStrategy() (string, error)

// GetColorBool returns color setting for "name" (e.g. color.diff).
GetColorBool(name string) (bool, error)

// GetColor returns color configured for "name" (e.g. color.diff.new).
GetColor(name, defaultValue string) (string, error)

// HasUncommittedChanges returns true if there are local, uncommitted changes.
HasUncommittedChanges() (bool, error)

Expand Down