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.

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

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion commands/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func listReviews(repo repository.Repo, args []string) error {
return nil
}
for _, r := range reviews {
output.PrintSummary(&r, &repo)
output.PrintSummary(&r)
}
return nil
}
Expand Down
52 changes: 33 additions & 19 deletions commands/output/output.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,30 +87,44 @@ func getStatusString(r *review.Summary) string {
return "rejected"
}

// PrintSummary prints a single-line summary of a review.
func PrintSummary(r *review.Summary, repo *repository.Repo) {
var use_color bool = false
func getColoredStatusString(repo repository.Repo, statusString string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
}

if repo != nil {
use_color = (*repo).GetColorBool("color.appraise")
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
}

statusString := getStatusString(r)
indentedDescription := strings.Replace(r.Request.Description, "\n", "\n ", -1)
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
	}

var colorOn string = ""
var colorOff string = ""
if use_color {
defaultColor, _ := default_color[statusString]
colorOn = (*repo).GetColor(
fmt.Sprintf("color.appraise.%s", statusString),
defaultColor,
)
colorOff = "\033[00m"
if err != nil {
return statusString
}
coloredStatusString := fmt.Sprintf("%s%s%s", colorOn, statusString, colorOff)

fmt.Printf(reviewSummaryTemplate, coloredStatusString, r.Revision, indentedDescription)
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,
getColoredStatusString(r.Repo, statusString),
r.Revision,
indentedDescription,
)
}

// reformatTimestamp takes a timestamp string of the form "0123456789" and changes it
Expand Down Expand Up @@ -213,7 +227,7 @@ func printComments(r *review.Review) error {

// PrintDetails prints a multi-line overview of a review, including all comments.
func PrintDetails(r *review.Review) error {
PrintSummary(r.Summary, nil)
PrintSummary(r.Summary)
fmt.Printf(reviewDetailsTemplate, r.Request.ReviewRef, r.Request.TargetRef,
strings.Join(r.Request.Reviewers, ", "),
r.Request.Requester, r.GetBuildStatusMessage())
Expand Down
37 changes: 6 additions & 31 deletions repository/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -986,36 +986,11 @@ func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
return updatedReviews, nil
}

func (repo *GitRepo) GetColorBool(name string) bool {
ok := repo.runGitCommandInline("config", "--get-colorbool", name)
return (ok == nil)
}

func (repo *GitRepo) GetColor(name, default_value string) string {
var res string
var ok error
if default_value == "" {
res, ok = repo.runGitCommand(
"config",
"--type=color",
"-z",
"--get",
name,
)
} else {
res, ok = repo.runGitCommand(
"config",
"--type=color",
"-z",
"--default",
default_value,
"--get",
name,
)
}
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.

}

if ok != nil {
return ""
}
return res
func (repo *GitRepo) GetColor(name, defaultValue string) (string, error) {
return repo.runGitCommand("config", "--get-color", name, defaultValue)
}
8 changes: 4 additions & 4 deletions repository/mock_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -612,10 +612,10 @@ func (repo *mockRepoForTest) FetchAndReturnNewReviewHashes(remote, notesRefPatte
return nil, nil
}

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

func (repo *mockRepoForTest) GetColor(name, default_value string) string {
return ""
func (repo *mockRepoForTest) GetColor(name, default_value string) (string, error) {
return "", nil
}
9 changes: 6 additions & 3 deletions repository/repo.go
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, default_value string) (string, error)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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)

Expand Down Expand Up @@ -218,7 +224,4 @@ type Repo interface {
// changed because the _names_ of these files correspond to the revisions
// they point to.
FetchAndReturnNewReviewHashes(remote, notesRefPattern, archiveRefPattern string) ([]string, error)

GetColorBool(name string) bool
GetColor(name, default_value string) string
}