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

Support color output for the added/deleted lines in diff mode #388

Merged
merged 4 commits into from Jun 14, 2019

Conversation

mingrammer
Copy link
Contributor

@mingrammer mingrammer commented Jun 12, 2019

This PR supports color output for the added/deleted lines in diff mode.

Current:

Screen Shot 2019-06-13 at 12 07 31 AM

When PR is merged:

Screen Shot 2019-06-13 at 12 07 09 AM

@mvdan
Copy link
Owner

mvdan commented Jun 12, 2019

Thanks for the PR. However, I disagree with always using colored output. Tools like diff will only do so when stdout is a terminal, for example. Printing color codes always isn't a good idea; for example, some people avoid them via TERM=dumb.

I'll take a look at an alternative solution.

@mingrammer
Copy link
Contributor Author

mingrammer commented Jun 12, 2019

Thank you for feedback. So how about using fatih/color? This package works well even if TERM=dumb set. (If set, it does not add the color codes around the text) See this.

As you see, the author of this library has stopped maintaining, but it is already "done". (no need further maintenance)

@mvdan
Copy link
Owner

mvdan commented Jun 12, 2019

I'm still not convinced. That would be an extra dependency, which itself also pulls in another two dependencies, all just so we could have a bit of logic to color a few lines.

But I do see the use of colored diffs. I'm ok with merging something like this, as long as colors are only enabled when:

  • TERM is not dumb
  • out is an *os.File, and IsTerminal returns true

I'll also leave a code review shortly.

@@ -322,16 +326,34 @@ func diffBytes(b1, b2 []byte, path string) ([]byte, error) {
// as diff will return non-zero if the files differ.
return nil, err
}

output := make([][]byte, 0)
Copy link
Owner

@mvdan mvdan Jun 12, 2019

Choose a reason for hiding this comment

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

use a bytes.Buffer instead, and then the functions can use fmt.Fprint(w, ...). this also means we won't need to use bytes.Join.

@@ -48,6 +48,10 @@ var (
in io.Reader = os.Stdin
out io.Writer = os.Stdout

ansiFgRed = []byte("\u001b[31m")
ansiFgGreen = []byte("\u001b[32m")
ansiReset = []byte("\u001b[0m")
Copy link
Owner

@mvdan mvdan Jun 12, 2019

Choose a reason for hiding this comment

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

once you switch to a buffer, these can be simply strings

-
`
%s
`, markDeleted([]byte("- foo")), markAdded([]byte("+foo")), markDeleted([]byte("-")))
Copy link
Owner

@mvdan mvdan Jun 12, 2019

Choose a reason for hiding this comment

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

Now that we'll have diff with and without colors, you could make color a global bool like diff, and have two subtests:

Diff, which is the old test as-is
DiffColored, which sets color = true, and ensures the escape codes are present.

Also, don't use fmt.Sprintf for the expected output. Just write the entire string as a single literal, escape codes and all.

@mingrammer
Copy link
Contributor Author

mingrammer commented Jun 12, 2019

Thanks for the detailed reviews. I’ll fix and improve them today!

@mingrammer
Copy link
Contributor Author

mingrammer commented Jun 14, 2019

I fixed codes by reflecting the reviews.

Copy link
Owner

@mvdan mvdan left a comment

Sorry, but see the os.File and TERM=dumb logic I explained earlier. This can't be a flag either.

@@ -26,6 +26,7 @@ var (
simple = flag.Bool("s", false, "")
find = flag.Bool("f", false, "")
diff = flag.Bool("d", false, "")
color = flag.Bool("c", false, "")
Copy link
Owner

@mvdan mvdan Jun 14, 2019

Choose a reason for hiding this comment

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

sorry, but no flag - this should be on by default on a terminal, and it can be turned off with TERM=dumb

Copy link
Contributor Author

@mingrammer mingrammer Jun 14, 2019

Choose a reason for hiding this comment

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

Oh, sorry I missed that comment. I fixed it.

mvdan
mvdan approved these changes Jun 14, 2019
Copy link
Owner

@mvdan mvdan left a comment

Thanks!

@mvdan mvdan merged commit 19eb31b into mvdan:master Jun 14, 2019
1 check passed
@mingrammer mingrammer deleted the diff-color branch Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants