Address feedback from pellared

[martinlindhe]

• NO_COLOR env is respected in color_windows.go
• fixes indent

[martinlindhe]

Please wait a minute with merging this. Verifying on my windows machine that NO_COLOR=1 works as expected.

EDIT: Tested working

[pellared]

Suggested change

	if err := windows.GetConsoleMode(out, &outMode); err == nil || NoColor {
	if err := windows.GetConsoleMode(out, &outMode); err != nil {

see #186 (comment)

[pellared]

I guess it would be better to add opt-out code as a separate PR 😉

[fatih]

I wrote more here, but let's remove NoColor integration. It wouldn't work during runtime. See my comment: #186 (comment)

Also we want to return if GetConsoleMode doesn't work, so err should be non-nil

Suggested change

	if err := windows.GetConsoleMode(out, &outMode); err == nil || NoColor {
	if err := windows.GetConsoleMode(out, &outMode); err != nil {

[martinlindhe]

Also we want to return if GetConsoleMode doesn't work, so err should be non-nil.

D'oh. Fixed.

So, should we just abandon this PR then, since it just attempts the NoColor integration?

[martinlindhe]

Tested NO_COLOR override on windows (works).

package main

import (
	"github.com/fatih/color"
)

func main() {
	c := color.New(color.FgCyan).Add(color.Underline)
	c.Println("Prints cyan text with an underline.")
}

[fatih]

@martinlindhe thank you. So the issue with no_color is, as I said happens during runtime. Here is an example it would fail:

package main

import (
	"github.com/fatih/color"
)

func main() {
	c := color.New(color.FgCyan).Add(color.Underline)
	c.Println("Prints cyan text with an underline.")

	color.NoColor = false 
	c.Println("Prints cyan text with an underline.") // this is still not colored
}

[martinlindhe]

@martinlindhe thank you. So the issue with no_color is, as I said happens during runtime. Here is an example it would fail:

package main

import (
	"github.com/fatih/color"
)

func main() {
	c := color.New(color.FgCyan).Add(color.Underline)
	c.Println("Prints cyan text with an underline.")

	color.NoColor = false 
	c.Println("Prints cyan text with an underline.") // this is still not colored
}

Well, this example also fails under Linux with fatih/color v1.14.1 (both lines are colored, or both are non-colored if NO_COLOR=1), so is it fair to try to address it in a corner-case for only Windows?

[fatih]

@martinlindhe I think we just discovered a bug here. I'll push a fix. Looks like when you use an environment variable, it sticks and won't change in runtime afterwards. But the code should work as I described.

[martinlindhe]

@fatih Cool. I'll take a step away from this. Please use these changes or drop this PR as you see fit. Thanks for your help!

[fatih]

Closing this, as I merged #188 and the no color approach needs to be rethought. I'm going to look into it. Thanks for the work so far @martinlindhe and @pellared

[martinlindhe]

Thank you @fatih!