-
Notifications
You must be signed in to change notification settings - Fork 144
Add coloring for review state in "git appraise list" #93
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.
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
"strings" | ||
"time" | ||
|
||
"github.com/google/git-appraise/repository" | ||
"github.com/google/git-appraise/review" | ||
) | ||
|
||
|
@@ -52,6 +53,15 @@ status: %s | |
// Number of lines of context to print for inline comments | ||
contextLineCount = 5 | ||
) | ||
var default_color = map[string]string{ | ||
"tbr": "red white bold blink", | ||
"pending": "cyan", | ||
"submitted": "yellow", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -77,11 +87,44 @@ func getStatusString(r *review.Summary) string { | |
return "rejected" | ||
} | ||
|
||
func getColoredStatusString(repo repository.Repo, statusString string) string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's call this "getColorStatusString". |
||
useColor, err := repo.GetColorBool("color.appraise") | ||
if err != nil || !useColor { | ||
return statusString | ||
} | ||
|
||
var colorOn string | ||
var colorOff string | ||
|
||
defaultColor, _ := default_color[statusString] | ||
colorOn, err = repo.GetColor( | ||
fmt.Sprintf("color.appraise.%s", statusString), | ||
defaultColor, | ||
) | ||
if err != nil { | ||
return statusString | ||
} | ||
|
||
colorOff, err = repo.GetColor("", "reset") | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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, | ||
getColoredStatusString(r.Repo, statusString), | ||
r.Revision, | ||
indentedDescription, | ||
) | ||
} | ||
|
||
// reformatTimestamp takes a timestamp string of the form "0123456789" and changes it | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, default_value string) (string, error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be "defaultValue" rather than "default_value" |
||
|
||
// HasUncommittedChanges returns true if there are local, uncommitted changes. | ||
HasUncommittedChanges() (bool, error) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be "defaultColor" rather than "default_color" (i.e. we use mixed case rather than underscores)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my 8-hours-per-day coding style seems to have become my second nature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I'll call it defaultColors just in case variable name shadowing is a no-no.