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

llorens · 2019-01-13T22:26:04Z

This commit adds different color highlighting to different review states
in the output of "git-appraise list".

The default colors are:

abandon: magenta,
accepted: green,
danger: yellow red bold blink,
pending: cyan,
rejected: yellow red bold strike,
submitted: yellow,
tbr: red white bold blink.

They can be modified by the user by setting the corresponding
configuration options in the color.appraise section. E.g. to use magenta
for the "pending" state, set color.appraise.pending to "magenta":

 [color "appraise"]
     pending = magenta

This commit adds different color highlighting to different review states in the output of "git-appraise list". The default colors are: - abandon: magenta, - accepted: green, - danger: yellow red bold blink, - pending: cyan, - rejected: yellow red bold strike, - submitted: yellow, - tbr: red white bold blink. They can be modified by the user by setting the corresponding configuration options in the color.appraise section. E.g. to use magenta for the "pending" state, set color.appraise.pending to "magenta": [color "appraise"] pending = magenta

googlebot · 2019-01-13T22:26:09Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

llorens · 2019-01-13T22:26:16Z

Hi,
I just recently stumbled upon git-appraise and it immediately seemed a brilliant idea to me. I can imagine I could use it e.g. for one-on-one collaboration/tutoring for junior members of my team, but...
The junior members usually have little experience with git and so I would like to work a bit on the usability of git-appraise before I give it a shot.
One of the areas where I think git-appraise could be improved is output coloring. I put together some code to add colors to the different states a code review can be in. Could you have a look before I invest more time in supporting other places? This is the first time I've been writing go and the language still feels very awkward to me, so I'll be grateful for any comments!
Thanks!

ojarjur · 2019-01-14T21:14:58Z

@llorens Thanks for taking this on!

I hope git-appraise does work for your use case, and if you hit any issues, please let us know.

We do need a signed CLA before we can accept any of your changes, though.

The comment from "googlebot" above gives the URL you would need to go to in order to sign it.

Thanks.

llorens · 2019-01-14T21:51:00Z

OK, just signed the CLA.

googlebot · 2019-01-14T21:51:03Z

CLAs look good, thanks!

ojarjur

Thanks so much for taking the time to put this together!

I do have some feedback, but I think the general approach you are taking makes sense.

Just FYI, when you want to update your PR there is no need to rebase your commits; we like to keep all changes in the repo history since the goal is to track code reviews with git itself.

Again, thanks for your help here!

ojarjur · 2019-01-15T01:56:47Z

repository/repo.go

@@ -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


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)

ojarjur · 2019-01-15T01:57:31Z

repository/repo.go

@@ -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
+	GetColor(name, default_value string) string


We use camel case for variables instead of snake case.

i.e. defaultValue rather than default_value

ojarjur · 2019-01-15T02:18:45Z

commands/output/output.go

@@ -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) {


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

ojarjur · 2019-01-15T02:29:25Z

commands/output/output.go

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


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 }

ojarjur · 2019-01-15T02:34:24Z

repository/git.go

+	var res string
+	var ok error
+	if default_value == "" {
+		res, ok = repo.runGitCommand(


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

llorens · 2019-01-16T22:53:40Z

repository/git.go

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


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.

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.

ojarjur

This is looking really good, thanks for all of your efforts so far!

I do have some minor comments, mostly about naming, but once those are addressed this should be good to go.

ojarjur · 2019-01-16T23:08:36Z

commands/output/output.go

@@ -52,6 +53,15 @@ status: %s
 	// Number of lines of context to print for inline comments
 	contextLineCount = 5
 )
+var default_color = map[string]string{


this should be "defaultColor" rather than "default_color" (i.e. we use mixed case rather than underscores)

Sorry, my 8-hours-per-day coding style seems to have become my second nature.

BTW, I'll call it defaultColors just in case variable name shadowing is a no-no.

ojarjur · 2019-01-16T23:16:55Z

commands/output/output.go

@@ -77,11 +87,44 @@ func getStatusString(r *review.Summary) string {
 	return "rejected"
 }

+func getColoredStatusString(repo repository.Repo, statusString string) string {


Let's call this "getColorStatusString".

ojarjur · 2019-01-16T23:20:06Z

commands/output/output.go

+var default_color = map[string]string{
+	"tbr": "red white bold blink",
+	"pending": "cyan",
+	"submitted": "yellow",


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".

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",

That mapping sounds good to me.

ojarjur · 2019-01-16T23:20:59Z

repository/repo.go

+	GetColorBool(name string) (bool, error)
+
+	// GetColor returns color configured for "name" (e.g. color.diff.new).
+	GetColor(name, default_value string) (string, error)


This should be "defaultValue" rather than "default_value"

llorens · 2019-01-17T22:16:59Z

I'd wait with merging this. git appraise list -a got noticeably slower as a consequence of this change because git config is called twice for every review listed to get the coloring and color reset sequences. If you have a good idea how to solve this, I'll be happy to hear.
Maybe adding output.GetColorSettings() which would return a map and then passing this map to output.PrintSummaryWithColor()? Unless it's OK to add a new parameter to output.PrintSummary()?

ojarjur · 2019-01-18T18:06:07Z

@llorens Thanks for the warning.

My suggestion matches with what you mentioned; pass a map of colors to a new PrintSummaryWithColor method.

We could then load that map once inside of the commands/list.go and commands/show.go files.

ojarjur reviewed Jan 15, 2019

View reviewed changes

Follow code review comments.

da84096

llorens commented Jan 16, 2019

View reviewed changes

ojarjur reviewed Jan 16, 2019

View reviewed changes

More naming problems.

fa20e5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

llorens commented Jan 13, 2019 •

edited

Loading

googlebot commented Jan 13, 2019

llorens commented Jan 13, 2019

ojarjur commented Jan 14, 2019

llorens commented Jan 14, 2019

googlebot commented Jan 14, 2019

ojarjur left a comment

ojarjur Jan 15, 2019

ojarjur Jan 15, 2019

ojarjur Jan 15, 2019

ojarjur Jan 15, 2019

ojarjur Jan 15, 2019

llorens Jan 16, 2019

ojarjur Jan 16, 2019

ojarjur left a comment

ojarjur Jan 16, 2019

llorens Jan 17, 2019

llorens Jan 17, 2019

ojarjur Jan 16, 2019

ojarjur Jan 16, 2019

llorens Jan 17, 2019

ojarjur Jan 18, 2019

ojarjur Jan 16, 2019

llorens commented Jan 17, 2019

ojarjur commented Jan 18, 2019

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

Are you sure you want to change the base?

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

Conversation

llorens commented Jan 13, 2019 • edited Loading

googlebot commented Jan 13, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

llorens commented Jan 13, 2019

ojarjur commented Jan 14, 2019

llorens commented Jan 14, 2019

googlebot commented Jan 14, 2019

ojarjur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojarjur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorens commented Jan 17, 2019

ojarjur commented Jan 18, 2019

llorens commented Jan 13, 2019 •

edited

Loading