#285 add color output for mage list #301

mirogta · 2020-04-25T23:10:46Z

I've chosen a cyan color, because it's a good contrast on a typically black terminal (better than e.g. blue) and it's neutral, as opposed to e.g. green, red, yellow.

Added one test to verify the colorized output. All other tests remain without the colorized output, because I unset the TERM env var, using resetTerm func. I've added a call to the resetTerm into the testmain func, but it wasn't enough and some tests were still failing so I've added an explicit call to the resetTerm to them as well.

natefinch

This needs to be configurable... I think we should default to off, since that's how it has worked in the past. I'd also like to offer configuration for what color for the labels.

I suggest making the config an environment variable - MAGEFILE_ENABLE_COLOR. Use strconv.ParseBool to detect if it's true or false (and false if not set or set to something weird). And then let people set MAGEFILE_TARGET_COLOR to something different if they don't want cyan.

Black: \u001b[30m
Red: \u001b[31m
Green: \u001b[32m
Yellow: \u001b[33m
Blue: \u001b[34m
Magenta: \u001b[35m
Cyan: \u001b[36m
White: \u001b[37m

(black or white might make sense if they have a colored background)

Maybe also the bright colors:

Bright Black: \u001b[30;1m
Bright Red: \u001b[31;1m
Bright Green: \u001b[32;1m
Bright Yellow: \u001b[33;1m
Bright Blue: \u001b[34;1m
Bright Magenta: \u001b[35;1m
Bright Cyan: \u001b[36;1m
Bright White: \u001b[37;1m

Oh, and since we're adding environment variables, we'll need to update the docs here:
https://github.com/magefile/mage/blob/master/site/content/environment/_index.en.md

Thanks! This is a cool feature that I'm sure people will enjoy (I'll definitely turn it on!)

natefinch · 2020-05-19T03:07:01Z

mage/template.go

+	// 	windows cmd.exe, powerShell.exe
+	terminalSupportsColor := func() bool {
+		envTerm := os.Getenv("TERM")
+		if strings.Contains(envTerm, "xterm") {


This kinda skeeves me out. Are we sure there are no existing terminals that include the substring xterm that don't support color? (I ask because I have no clue)

Yeah, the best I could find was that there's "xterm-mono". I've added a condition to disable terminal support if this is detected.

See rust-lang/cargo#7646

I can say from experience that this topic could be a separate study course 🙉
Here's a list of possible terminal identifiers from my color theme project for GNU dircolors (the tool that provides the color database for the GNU ls command by populating the LS_COLORS environment variable).
The list contains a lot more IDs and I'm sure it still misses some, e.g. new terminals like Alacritty bring their own new identifiers and terminfo definitions.
Anyway, maybe it's easier to only check for terminal IDs that are definitely known to not support colors and let all other IDs pass. This way the list could be extended from time to time when users report terminal IDs that are not checked yet. Since this is a opt-in feature it should not break anyone's output.

only check of terminal IDs that definitely known to not support colors and let all others IDs pass

Great idea! @natefinch any objections? The code would get significantly simpler.

// Terminals which don't support color: // TERM=vt100 // TERM=cygwin // TERM=xterm-mono var noColorTerms := [3]string{"vt100", "cygwin", "xterm-mono"}

Sorry, I missed this was a question for me. Yes, let's do that.

Cool - done, see the update.

I've used a map rather than a list for performance, so that I don't need to iterate over it and just use the terminal name as a key.

"It works on my machine" including all tests, but having some trouble with tests in Travis CI. Trying to find out what is the TERM set to...

Looking good now. Ready for a merge & release?

natefinch · 2020-05-19T03:22:29Z

Sorry this sat around so long, I can't believe it's been 23 days. Time no longer has meaning, I guess.

I love that you did this without requiring an import of a 3rd party library.

mirogta · 2020-05-19T10:58:37Z

Sorry this sat around so long, I can't believe it's been 23 days. Time no longer has meaning, I guess.

I love that you did this without requiring an import of a 3rd party library.

No problem at all, my time is also completely skewed - can't believe it's almost June! I'll crack on the suggestions :-) Thank you

…ET_COLOR env vars

- also handling xterm-mono, so that it forces non-color output

mirogta · 2020-05-19T23:50:36Z

@natefinch please have a look if it looks sensible and I'll add more tests around it. I know, it's not really TDD and so on, but it would be quite difficult to follow in this case while I was just exploring the best approach.

Please note I've left the new color.go file with a test there, even though it's not really used in any code (yet). It has helped me massively to get the code working, which I could then copy&paste with just minor modifications into the template.go.

I specifically wanted to use plain English names for the colors, as opposed to the cryptic ANSI codes. So that has expanded the code quite a bit. But I think it's worth it.

We don't want to add external dependencies, so I've copied toLower function from strings package there, so that the color names can be case insensitive. Luckily, we do use strings in the generated template (via template.go), so the code there is much simpler.

Exploratory manual test scenarios:

MAGEFILE_ENABLE_COLOR=0
# MAGEFILE_TARGET_COLOR not set - defaults to "cyan"
go run main.go -l

MAGEFILE_ENABLE_COLOR=1
# MAGEFILE_TARGET_COLOR not set - defaults to "cyan"
go run main.go -l

MAGEFILE_ENABLE_COLOR=1
MAGEFILE_TARGET_COLOR="red"
go run main.go -l

MAGEFILE_ENABLE_COLOR=1
MAGEFILE_TARGET_COLOR="pink" # not recognised, so defaults to cyan
go run main.go -l

MAGEFILE_ENABLE_COLOR=0
MAGEFILE_TARGET_COLOR="blue" # color disabled, so it's ignored
go run main.go -l

natefinch · 2020-06-10T14:53:39Z

This looks great. Lemme see if anyone else wants to get eyes on it.

* use a list of specfic terminals which don't support color

natefinch

LGTM with a few nitpicky changes.

mage/main_test.go

mg/runtime.go

magefile#285 add color output for mage list

9495507

mirogta mentioned this pull request Apr 25, 2020

Add color output for mage -l targets #285

Closed

natefinch requested changes May 19, 2020

View reviewed changes

mirogta added 2 commits May 20, 2020 00:41

added color configuration via MAGEFILE_ENABLE_COLOR and MAGEFILE_TARG…

0a042a3

…ET_COLOR env vars

fixed template

9a98a25

- also handling xterm-mono, so that it forces non-color output

mirogta added 3 commits May 20, 2020 21:41

typo

90314f9

more typos

d1da6c9

fix test

50d2587

mirogta requested a review from natefinch June 8, 2020 15:31

mirogta added 3 commits July 8, 2020 16:15

simplify color terminal detection

a894948

* use a list of specfic terminals which don't support color

fix: color output test in CI

d4fdf58

fix: test in CI

71bfbb0

arcticicestudio previously approved these changes Jul 8, 2020

View reviewed changes

natefinch previously approved these changes Jul 8, 2020

View reviewed changes

mage/main_test.go Outdated Show resolved Hide resolved

mg/runtime.go Outdated Show resolved Hide resolved

mg/runtime.go Outdated Show resolved Hide resolved

Update mage/main_test.go

7afad3c

natefinch dismissed stale reviews from arcticicestudio and themself via 7afad3c July 8, 2020 17:56

natefinch added 2 commits July 8, 2020 13:57

Update mg/runtime.go

a1f2a7c

Update mg/runtime.go

7254217

natefinch approved these changes Jul 8, 2020

View reviewed changes

natefinch merged commit 9a10961 into magefile:master Jul 8, 2020

mirogta deleted the color-output-list branch March 29, 2021 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#285 add color output for mage list #301

#285 add color output for mage list #301

mirogta commented Apr 25, 2020

natefinch left a comment

natefinch May 19, 2020

mirogta May 19, 2020 •

edited

Loading

arcticicestudio Jun 10, 2020 •

edited

Loading

mirogta Jun 10, 2020

natefinch Jul 7, 2020

mirogta Jul 8, 2020

mirogta Jul 8, 2020

mirogta Jul 8, 2020

natefinch commented May 19, 2020

mirogta commented May 19, 2020

mirogta commented May 19, 2020 •

edited

Loading

natefinch commented Jun 10, 2020

natefinch left a comment

#285 add color output for mage list #301

#285 add color output for mage list #301

Conversation

mirogta commented Apr 25, 2020

natefinch left a comment

Choose a reason for hiding this comment

natefinch May 19, 2020

Choose a reason for hiding this comment

mirogta May 19, 2020 • edited Loading

Choose a reason for hiding this comment

arcticicestudio Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

mirogta Jun 10, 2020

Choose a reason for hiding this comment

natefinch Jul 7, 2020

Choose a reason for hiding this comment

mirogta Jul 8, 2020

Choose a reason for hiding this comment

mirogta Jul 8, 2020

Choose a reason for hiding this comment

mirogta Jul 8, 2020

Choose a reason for hiding this comment

natefinch commented May 19, 2020

mirogta commented May 19, 2020

mirogta commented May 19, 2020 • edited Loading

natefinch commented Jun 10, 2020

natefinch left a comment

Choose a reason for hiding this comment

mirogta May 19, 2020 •

edited

Loading

arcticicestudio Jun 10, 2020 •

edited

Loading

mirogta commented May 19, 2020 •

edited

Loading