Repository: gin-gonic/gin
Pull request #1802 (2019-03-08 12:52:59)

IsTerm flag should not be affected by DisableConsoleColor method.
It is strange behavior that `DisableConsoleColor` turns the `IsTerm` flag false.

This PR contains the following changes.
 - `IsTerm` represents whether is the output destination console.
 - Default logger distinguishes color by using new method `IsOutputColor`.
 - If `ForceConsoleColor` and `DisableConsoleColor` are called together, Later one is valid.

**Note**
This change breaks backward compability, but `LogFormatterParams` and `ForceConsoleColor` is not released yet. This change affects only the user which is using master branch.

codecov[bot]: # [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=h1) Report
> Merging [#1802](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=desc) into [master](https://codecov.io/gh/gin-gonic/gin/commit/f7079a861e6db66385aae1d865b9c9bbe10d1bb1?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/gin-gonic/gin/pull/1802/graphs/tree.svg?width=650&token=Qpqxhwery8&height=150&src=pr)](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1802      +/-   ##
==========================================
+ Coverage   98.62%   98.62%   +<.01%     
==========================================
  Files          41       41              
  Lines        2113     2114       +1     
==========================================
+ Hits         2084     2085       +1     
  Misses         18       18              
  Partials       11       11
```


| [Impacted Files](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [logger.go](https://codecov.io/gh/gin-gonic/gin/pull/1802/diff?src=pr&el=tree#diff-bG9nZ2VyLmdv) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=footer). Last update [f7079a8...7f3e502](https://codecov.io/gh/gin-gonic/gin/pull/1802?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

thinkerou: About color I think that have only two case:
1. Not use color, even` isTerm=true`.
2. Use color, need `isTerm=true`.
so we only need one switch to indicate whether use color.
Default use color when isTerm=true, or not use.
sairoutine: @thinkerou 
Your suggested usecases are true. Therefore, the following usecases are seldom occured.

> - If `ForceConsoleColor` and `DisableConsoleColor` are called together, Later one is valid.

This PR's purposes are two things.
 - It is strange behavior that `DisableConsoleColor` turns the `IsTerm` flag false.
 - `disableColor` and `forceColor` are redundant variables. To consolidate into `consoleColorMode` makes gin's code more readable.
thinkerou: @sairoutine I hope simple.
gin default use color on Term mode.
If user calls `gin.DisableConsoleColor()`, not use color.

sairoutine: @thinkerou 
>gin default use color on Term mode.
>If user calls gin.DisableConsoleColor(), not use color.

This PR does not change this behavior.

If the user does not call `gin.DisableConsoleColor()` or `gin.ForceConsoleColor()`, gin decides to use color by Term mode.

If the user calls `gin.DisableConsoleColor()`, gin does not use color whatever `isTerm=true` or `isTerm=false`.
thinkerou: @sairoutine 
>This PR does not change this behavior.

Yes, I know. So I hope change it release gin1.4 before. do you think? thanks!
sairoutine: @thinkerou 
Thanks! I also think this PR should be merged before gin 1.4 release.

thinkerou: @sairoutine I hope the pull request change to:
> gin default use color on Term mode.
> If user calls gin.DisableConsoleColor(), not use color.
can you recommit one patch? thanks!
sairoutine: @thinkerou 
It seems that this PR is according to this. Could you tell me what should I do?

>gin default use color on Term mode.
>If user calls gin.DisableConsoleColor(), not use color.
>can you recommit one patch? thanks!
thinkerou: @sairoutine OK, I see #1724 content about `forceConsoleColor`, you not need to do what. Thanks!
sairoutine: Thanks! Yes, This PR is related to #1724.

Patch:
diff --git a/logger.go b/logger.go
index 2ecaed7..198a019 100644
--- a/logger.go
+++ b/logger.go
@@ -14,17 +14,24 @@ import (
 	"github.com/mattn/go-isatty"
 )
 
+type consoleColorModeValue int
+
+const (
+	autoColor consoleColorModeValue = iota
+	disableColor
+	forceColor
+)
+
 var (
-	green        = string([]byte{27, 91, 57, 55, 59, 52, 50, 109})
-	white        = string([]byte{27, 91, 57, 48, 59, 52, 55, 109})
-	yellow       = string([]byte{27, 91, 57, 48, 59, 52, 51, 109})
-	red          = string([]byte{27, 91, 57, 55, 59, 52, 49, 109})
-	blue         = string([]byte{27, 91, 57, 55, 59, 52, 52, 109})
-	magenta      = string([]byte{27, 91, 57, 55, 59, 52, 53, 109})
-	cyan         = string([]byte{27, 91, 57, 55, 59, 52, 54, 109})
-	reset        = string([]byte{27, 91, 48, 109})
-	disableColor = false
-	forceColor   = false
+	green            = string([]byte{27, 91, 57, 55, 59, 52, 50, 109})
+	white            = string([]byte{27, 91, 57, 48, 59, 52, 55, 109})
+	yellow           = string([]byte{27, 91, 57, 48, 59, 52, 51, 109})
+	red              = string([]byte{27, 91, 57, 55, 59, 52, 49, 109})
+	blue             = string([]byte{27, 91, 57, 55, 59, 52, 52, 109})
+	magenta          = string([]byte{27, 91, 57, 55, 59, 52, 53, 109})
+	cyan             = string([]byte{27, 91, 57, 55, 59, 52, 54, 109})
+	reset            = string([]byte{27, 91, 48, 109})
+	consoleColorMode = autoColor
 )
 
 // LoggerConfig defines the config for Logger middleware.
@@ -62,8 +69,8 @@ type LogFormatterParams struct {
 	Path string
 	// ErrorMessage is set if error has occurred in processing the request.
 	ErrorMessage string
-	// IsTerm shows whether does gin's output descriptor refers to a terminal.
-	IsTerm bool
+	// isTerm shows whether does gin's output descriptor refers to a terminal.
+	isTerm bool
 	// BodySize is the size of the Response Body
 	BodySize int
 	// Keys are the keys set on the request's context.
@@ -115,10 +122,15 @@ func (p *LogFormatterParams) ResetColor() string {
 	return reset
 }
 
+// IsOutputColor indicates whether can colors be outputted to the log.
+func (p *LogFormatterParams) IsOutputColor() bool {
+	return consoleColorMode == forceColor || (consoleColorMode == autoColor && p.isTerm)
+}
+
 // defaultLogFormatter is the default log format function Logger middleware uses.
 var defaultLogFormatter = func(param LogFormatterParams) string {
 	var statusColor, methodColor, resetColor string
-	if param.IsTerm {
+	if param.IsOutputColor() {
 		statusColor = param.StatusCodeColor()
 		methodColor = param.MethodColor()
 		resetColor = param.ResetColor()
@@ -137,12 +149,12 @@ var defaultLogFormatter = func(param LogFormatterParams) string {
 
 // DisableConsoleColor disables color output in the console.
 func DisableConsoleColor() {
-	disableColor = true
+	consoleColorMode = disableColor
 }
 
 // ForceConsoleColor force color output in the console.
 func ForceConsoleColor() {
-	forceColor = true
+	consoleColorMode = forceColor
 }
 
 // ErrorLogger returns a handlerfunc for any error type.
@@ -199,9 +211,8 @@ func LoggerWithConfig(conf LoggerConfig) HandlerFunc {
 
 	isTerm := true
 
-	if w, ok := out.(*os.File); (!ok ||
-		(os.Getenv("TERM") == "dumb" || (!isatty.IsTerminal(w.Fd()) && !isatty.IsCygwinTerminal(w.Fd()))) ||
-		disableColor) && !forceColor {
+	if w, ok := out.(*os.File); !ok || os.Getenv("TERM") == "dumb" ||
+		(!isatty.IsTerminal(w.Fd()) && !isatty.IsCygwinTerminal(w.Fd())) {
 		isTerm = false
 	}
 
@@ -228,7 +239,7 @@ func LoggerWithConfig(conf LoggerConfig) HandlerFunc {
 		if _, ok := skip[path]; !ok {
 			param := LogFormatterParams{
 				Request: c.Request,
-				IsTerm:  isTerm,
+				isTerm:  isTerm,
 				Keys:    c.Keys,
 			}
 


Test patch:
diff --git a/logger_test.go b/logger_test.go
index 3623137..11a859e 100644
--- a/logger_test.go
+++ b/logger_test.go
@@ -240,7 +240,7 @@ func TestDefaultLogFormatter(t *testing.T) {
 		Method:       "GET",
 		Path:         "/",
 		ErrorMessage: "",
-		IsTerm:       false,
+		isTerm:       false,
 	}
 
 	termTrueParam := LogFormatterParams{
@@ -251,7 +251,7 @@ func TestDefaultLogFormatter(t *testing.T) {
 		Method:       "GET",
 		Path:         "/",
 		ErrorMessage: "",
-		IsTerm:       true,
+		isTerm:       true,
 	}
 
 	assert.Equal(t, "[GIN] 2018/12/07 - 09:11:42 | 200 |            5s |     20.20.20.20 | GET      /\n", defaultLogFormatter(termFalseParam))
@@ -296,6 +296,39 @@ func TestResetColor(t *testing.T) {
 	assert.Equal(t, string([]byte{27, 91, 48, 109}), p.ResetColor())
 }
 
+func TestIsOutputColor(t *testing.T) {
+	// test with isTerm flag true.
+	p := LogFormatterParams{
+		isTerm: true,
+	}
+
+	consoleColorMode = autoColor
+	assert.Equal(t, true, p.IsOutputColor())
+
+	ForceConsoleColor()
+	assert.Equal(t, true, p.IsOutputColor())
+
+	DisableConsoleColor()
+	assert.Equal(t, false, p.IsOutputColor())
+
+	// test with isTerm flag false.
+	p = LogFormatterParams{
+		isTerm: false,
+	}
+
+	consoleColorMode = autoColor
+	assert.Equal(t, false, p.IsOutputColor())
+
+	ForceConsoleColor()
+	assert.Equal(t, true, p.IsOutputColor())
+
+	DisableConsoleColor()
+	assert.Equal(t, false, p.IsOutputColor())
+
+	// reset console color mode.
+	consoleColorMode = autoColor
+}
+
 func TestErrorLogger(t *testing.T) {
 	router := New()
 	router.Use(ErrorLogger())
@@ -358,14 +391,20 @@ func TestLoggerWithConfigSkippingPaths(t *testing.T) {
 
 func TestDisableConsoleColor(t *testing.T) {
 	New()
-	assert.False(t, disableColor)
+	assert.Equal(t, autoColor, consoleColorMode)
 	DisableConsoleColor()
-	assert.True(t, disableColor)
+	assert.Equal(t, disableColor, consoleColorMode)
+
+	// reset console color mode.
+	consoleColorMode = autoColor
 }
 
 func TestForceConsoleColor(t *testing.T) {
 	New()
-	assert.False(t, forceColor)
+	assert.Equal(t, autoColor, consoleColorMode)
 	ForceConsoleColor()
-	assert.True(t, forceColor)
+	assert.Equal(t, forceColor, consoleColorMode)
+
+	// reset console color mode.
+	consoleColorMode = autoColor
 }