search

pkg / diff

BSD 3-Clause "New" or "Revised" License
88 stars 9 forks source link

Add a WriteOpt to color the output for a terminal #10

Closed mvdan closed 5 years ago

mvdan commented 5 years ago

I think this will be a common need for user-facing tools; people generally expect diffs to be readable without having to look at the + or - prefixes of the unified diff.

I think we can begin with a TerminalColor that always uses the red/green colors. If anyone wants to control when this is used, e.g. via isatty, they can do that in the calling code. This way, if anyone requests other color formats such as HTML, we aren't using the confusing Color option name.

I've written this on my own code that uses pkg/diff, so I'm happy to send a PR if this seems like a good idea.

mvdan commented 5 years ago

A more powerful, but probably overkill version of this would be a func that would transform lines before they're written out. In our case, a leading + would wrap the line in green color for a terminal or HTML, for example.

But for pkg/diff users, they'd very probably prefer a simple option they can use directly for the common cases.

josharian commented 5 years ago

This is definitely a good idea.

A TerminalColor WriteOpt SGTM. Maybe also (eventually) an Intraline WriteOpt to use color highlighting to display intraline diffs.

Implementation detail: What should pkg/diff do if the user requests TerminalColor but TERM=dumb envvar is set?

A PR would be lovely. Or I'll get it to it myself eventually.

mvdan commented 5 years ago

Initially, I think that the decision when or not to output color should be done by the user. This includes what flags or options the tool has, or TERM=dumb.

I guess we could be extra grumpy if someone is bypassing TERM=dumb; but what options do we have? Giving an error won't help the user at all, as they would get a failure instead of garbled, but at least semi-useful output. A panic is equally not great.

We want to complain to the developer, and they'd probably never see such an error/panic from us. If they were exhaustive enough to test their program with TERM=dumb, they'd probably have written the correct logic to begin with.

I think we can just add a godoc warning to keep TERM=dumb in mind, and to not always enable color.