Skip to content

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.

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

Open
llorens 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 1 commit
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
2 changes: 1 addition & 1 deletion commands/list.go
View file
Open in desktop
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)
output.PrintSummary(&r, &repo)
}
return nil
}
Expand Down
35 changes: 32 additions & 3 deletions 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 default_color = map[string]string{

@ojarjur ojarjur Jan 16, 2019

Copy link
Copy Markdown
Collaborator

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)

@llorens llorens Jan 17, 2019

Copy link
Copy Markdown
Contributor Author

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.

@llorens llorens Jan 17, 2019

Copy link
Copy Markdown
Contributor Author

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.

"tbr": "red white bold blink",
"pending": "cyan",
"submitted": "yellow",

@ojarjur ojarjur Jan 16, 2019

Copy link
Copy Markdown
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".

@llorens llorens Jan 17, 2019

Copy link
Copy Markdown
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",

@ojarjur ojarjur Jan 18, 2019

Copy link
Copy Markdown
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 @@ -78,10 +88,29 @@ func getStatusString(r *review.Summary) string {
}

// PrintSummary prints a single-line summary of a review.
func PrintSummary(r *review.Summary) {
func PrintSummary(r *review.Summary, repo *repository.Repo) {

@ojarjur ojarjur Jan 15, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The review.Summary type holds a reference to the repo, so you don't have to pass it in here. Instead, you can just reference r.Repo.

Conversely, adding in the additional reference to the repo would be a change to an exported UI, which might break people using git-appraise as a library.

One additional aside; the repository.Repo type is an interface, so you would not normally pass it around as a pointer (but that will become a moot point after the switch to using r.Repo).

var use_color bool = false

if repo != nil {
use_color = (*repo).GetColorBool("color.appraise")
}

statusString := getStatusString(r)
indentedDescription := strings.Replace(r.Request.Description, "\n", "\n ", -1)
fmt.Printf(reviewSummaryTemplate, statusString, r.Revision, indentedDescription)

@ojarjur ojarjur Jan 15, 2019

Copy link
Copy Markdown
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"
}
coloredStatusString := fmt.Sprintf("%s%s%s", colorOn, statusString, colorOff)

fmt.Printf(reviewSummaryTemplate, coloredStatusString, r.Revision, indentedDescription)
}

// reformatTimestamp takes a timestamp string of the form "0123456789" and changes it
Expand Down Expand Up @@ -184,7 +213,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)
PrintSummary(r.Summary, nil)
fmt.Printf(reviewDetailsTemplate, r.Request.ReviewRef, r.Request.TargetRef,
strings.Join(r.Request.Reviewers, ", "),
r.Request.Requester, r.GetBuildStatusMessage())
Expand Down
34 changes: 34 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,37 @@ 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(

@ojarjur ojarjur Jan 15, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In this case I would recommend building the command args first, appending to them if the default is provided, and then only having one line that invokes the command.

e.g.

	args := []string{"config", "--type=color", "-z"}
	if defaultValue != "" {
		args = append(args, "--default", defaultValue)
	}
	args = append(args, "--get", name)
	res, err := repo.runGitCommand(args...)

"config",
"--type=color",
"-z",
"--get",
name,
)
} else {
res, ok = repo.runGitCommand(
"config",
"--type=color",
"-z",
"--default",
default_value,
"--get",
name,
)
}

if ok != nil {
return ""
}
return res
}
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 {
return false
}

func (repo *mockRepoForTest) GetColor(name, default_value string) string {
return ""
}
3 changes: 3 additions & 0 deletions repository/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,4 +218,7 @@ 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

@ojarjur ojarjur Jan 15, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two new methods need comments explaining what they do.

Additionally, these should probably go up with the rest of the config-fetching methods. I.E. just below the GetSubmitStrategy entry.

Additionally, they should return a tuple including an error, so that we can propagate outwards any errors in invoking the git command line tool

e.g. GetColorBool(name string) (bool, error) and GetColor(name, defaultValue string) (string, error)

GetColor(name, default_value string) string

@ojarjur ojarjur Jan 15, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We use camel case for variables instead of snake case.

i.e. defaultValue rather than default_value

}