Merge pull request #1218 from cloudflare/color

Unify coloring code

cmd/pint/logger.go

@@ -2,7 +2,6 @@ package main
 
 import (
 	"fmt"
-	"os"
 
 	"github.com/cloudflare/pint/internal/log"
 )
@@ -13,11 +12,6 @@ func initLogger(level string, noColor bool) error {
 		return fmt.Errorf("'%s' is not a valid log level", level)
 	}
 
-	nc := os.Getenv("NO_COLOR")
-	if nc != "" && nc != "0" {
-		noColor = true
-	}
-
 	log.Setup(l, noColor)
 
 	return nil

cmd/pint/main.go

@@ -53,6 +53,7 @@ func newApp() *cli.App {
 				Aliases: []string{"n"},
 				Value:   false,
 				Usage:   "Disable output colouring.",
+				EnvVars: []string{"NO_COLOR"},
 			},
 			&cli.StringSliceFlag{
 				Name:    disabledFlag,

cmd/pint/tests/0024_color_output.txt

@@ -6,51 +6,51 @@ cmp stderr stderr.txt
 -- stderr.txt --
 level=INFO msg="Loading configuration file" path=.pint.hcl
 level=INFO msg="Finding all rules to check" paths=["rules"]
-rules/0001.yml:2 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
- 2 |   expr: sum(rate(fl_cf_html_bytes_in[10m])) WITHOUT (colo_id, instance, node_type, region, node_status, job, colo_name)
-
-rules/0001.yml:6 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, use `without(instance, ...)`. (promql/aggregate)
- 6 |   expr: sum(irate(foo[3m])) WITHOUT (colo_id)
-
-rules/0003.yaml:11 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, use `without(instance, ...)`. (promql/aggregate)
- 11 |   expr: sum(foo) without(job)
-
-rules/0003.yaml:11 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
- 11 |   expr: sum(foo) without(job)
-
-rules/0003.yaml:14 Fatal: Prometheus failed to parse the query with this PromQL error: unexpected right parenthesis ')'. (promql/syntax)
- 14 |   expr: sum(foo) by ())
-
-rules/0003.yaml:22-25 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
- 22 |   expr: |
- 23 |     sum(
- 24 |       multiline
- 25 |     ) without(job, instance)
-
-rules/0003.yaml:28-31 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, use `without(instance, ...)`. (promql/aggregate)
- 28 |   expr: |
- 29 |     sum(sum) without(job)
- 30 |     +
- 31 |     sum(sum) without(job)
-
-rules/0003.yaml:28-31 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
- 28 |   expr: |
- 29 |     sum(sum) without(job)
- 30 |     +
- 31 |     sum(sum) without(job)
-
-rules/0003.yaml:34-37 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
- 34 |   expr: >-
- 35 |     sum(
- 36 |       multiline2
- 37 |     ) without(job, instance)
-
-rules/0003.yaml:40 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, remove instance from `by()`. (promql/aggregate)
- 40 |   expr: sum(byinstance) by(instance)
-
-rules/0003.yaml:40 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, use `by(job, ...)`. (promql/aggregate)
- 40 |   expr: sum(byinstance) by(instance)
-
+rules/0001.yml:2 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
+ 2 |   expr: sum(rate(fl_cf_html_bytes_in[10m])) WITHOUT (colo_id, instance, node_type, region, node_status, job, colo_name)
+
+rules/0001.yml:6 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, use `without(instance, ...)`. (promql/aggregate)
+ 6 |   expr: sum(irate(foo[3m])) WITHOUT (colo_id)
+
+rules/0003.yaml:11 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, use `without(instance, ...)`. (promql/aggregate)
+ 11 |   expr: sum(foo) without(job)
+
+rules/0003.yaml:11 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
+ 11 |   expr: sum(foo) without(job)
+
+rules/0003.yaml:14 Fatal: Prometheus failed to parse the query with this PromQL error: unexpected right parenthesis ')'. (promql/syntax)
+ 14 |   expr: sum(foo) by ())
+
+rules/0003.yaml:22-25 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
+ 22 |   expr: |
+ 23 |     sum(
+ 24 |       multiline
+ 25 |     ) without(job, instance)
+
+rules/0003.yaml:28-31 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, use `without(instance, ...)`. (promql/aggregate)
+ 28 |   expr: |
+ 29 |     sum(sum) without(job)
+ 30 |     +
+ 31 |     sum(sum) without(job)
+
+rules/0003.yaml:28-31 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
+ 28 |   expr: |
+ 29 |     sum(sum) without(job)
+ 30 |     +
+ 31 |     sum(sum) without(job)
+
+rules/0003.yaml:34-37 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, remove job from `without()`. (promql/aggregate)
+ 34 |   expr: >-
+ 35 |     sum(
+ 36 |       multiline2
+ 37 |     ) without(job, instance)
+
+rules/0003.yaml:40 Warning: `instance` label should be removed when aggregating `^colo(?:_.+)?:.+$` rules, remove instance from `by()`. (promql/aggregate)
+ 40 |   expr: sum(byinstance) by(instance)
+
+rules/0003.yaml:40 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, use `by(job, ...)`. (promql/aggregate)
+ 40 |   expr: sum(byinstance) by(instance)
+
 level=INFO msg="Problems found" Fatal=1 Warning=10
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
 -- rules/0001.yml --

go.mod

@@ -4,7 +4,6 @@ go 1.23.0
 
 require (
 	github.com/cespare/xxhash/v2 v2.3.0
-	github.com/fatih/color v1.18.0
 	github.com/gkampitakis/go-snaps v0.5.7
 	github.com/google/go-cmp v0.6.0
 	github.com/google/go-github/v63 v63.0.0
@@ -38,6 +37,7 @@ require (
 	github.com/dennwc/varint v1.0.0 // indirect
 	github.com/edsrzf/mmap-go v1.1.0 // indirect
 	github.com/facette/natsort v0.0.0-20181210072756-2cd4dd1e2dcb // indirect
+	github.com/fatih/color v1.18.0 // indirect
 	github.com/gkampitakis/ciinfo v0.3.0 // indirect
 	github.com/gkampitakis/go-diff v1.3.2 // indirect
 	github.com/go-logr/logr v1.4.2 // indirect
@@ -52,8 +52,6 @@ require (
 	github.com/kr/text v0.2.0 // indirect
 	github.com/kylelemons/godebug v1.1.0 // indirect
 	github.com/maruel/natural v1.1.1 // indirect
-	github.com/mattn/go-colorable v0.1.13 // indirect
-	github.com/mattn/go-isatty v0.0.20 // indirect
 	github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 // indirect
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect

go.sum

@@ -110,7 +110,6 @@ github.com/maruel/natural v1.1.1 h1:Hja7XhhmvEFhcByqDoHz9QZbkWey+COd9xWfCfn1ioo=
 github.com/maruel/natural v1.1.1/go.mod h1:v+Rfd79xlw1AgVBjbO0BEQmptqb5HvL/k9GRHB7ZKEg=
 github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=
 github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg=
-github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
 github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
 github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
 github.com/mitchellh/go-wordwrap v0.0.0-20150314170334-ad45545899c7 h1:DpOJ2HYzCv8LZP15IdmG+YdwD2luVPHITV96TkirNBM=
@@ -226,8 +225,6 @@ golang.org/x/sync v0.8.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
-golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.26.0 h1:KHjCJyddX0LoSTb3J+vWpupP9p0oznkqVk/IfjymZbo=
 golang.org/x/sys v0.26.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=

internal/log/handler.go

@@ -9,16 +9,8 @@ import (
 	"log/slog"
 	"strings"
 	"sync"
-)
 
-const (
-	dim         = 2
-	fgHiRed     = 91
-	fgHiYellow  = 93
-	fgHiBlue    = 94
-	fgHiMagenta = 95
-	fgHiCyan    = 96
-	fgHiWhite   = 97
+	"github.com/cloudflare/pint/internal/output"
 )
 
 type handler struct {
@@ -48,22 +40,22 @@ func (h *handler) Enabled(_ context.Context, level slog.Level) bool {
 func (h *handler) Handle(_ context.Context, record slog.Record) error {
 	buf := bytes.NewBuffer(make([]byte, 0, 128))
 
-	var lc int
+	var lc output.Color
 	switch record.Level {
 	case slog.LevelInfo:
-		lc = fgHiWhite
+		lc = output.White
 	case slog.LevelError:
-		lc = fgHiRed
+		lc = output.Red
 	case slog.LevelWarn:
-		lc = fgHiYellow
+		lc = output.Yellow
 	case slog.LevelDebug:
-		lc = fgHiMagenta
+		lc = output.Magenta
 	}
 	h.printKey(buf, "level")
 	h.printVal(buf, record.Level.String(), lc)
 	_, _ = buf.WriteRune(' ')
 	h.printKey(buf, "msg")
-	h.printVal(buf, record.Message, fgHiWhite)
+	h.printVal(buf, record.Message, output.White)
 
 	record.Attrs(func(attr slog.Attr) bool {
 		_, _ = buf.WriteRune(' ')
@@ -91,21 +83,14 @@ func (h *handler) WithGroup(_ string) slog.Handler {
 }
 
 func (h *handler) printKey(buf *bytes.Buffer, s string) {
-	_, _ = buf.WriteString(h.maybeWriteColor(s+"=", dim))
+	_, _ = buf.WriteString(output.MaybeColor(output.Dim, h.noColor, s+"="))
 }
 
-func (h *handler) printVal(buf *bytes.Buffer, s string, color int) {
+func (h *handler) printVal(buf *bytes.Buffer, s string, color output.Color) {
 	if !strings.HasPrefix(s, "[") && !strings.HasPrefix(s, "{") && strings.Contains(s, " ") {
 		s = "\"" + h.escaper.Replace(s) + "\""
 	}
-	_, _ = buf.WriteString(h.maybeWriteColor(s, color))
-}
-
-func (h *handler) maybeWriteColor(s string, color int) string {
-	if h.noColor {
-		return s
-	}
-	return fmt.Sprintf("\033[%dm%s\033[0m", color, s)
+	_, _ = buf.WriteString(output.MaybeColor(color, h.noColor, s))
 }
 
 func (h *handler) appendAttr(buf *bytes.Buffer, attr slog.Attr) {
@@ -118,14 +103,14 @@ func (h *handler) appendAttr(buf *bytes.Buffer, attr slog.Attr) {
 	case slog.KindAny:
 		switch attr.Value.Any().(type) {
 		case error:
-			h.printVal(buf, formatString(attr), fgHiRed)
+			h.printVal(buf, formatString(attr), output.Red)
 		default:
-			h.printVal(buf, formatAny(attr), fgHiCyan)
+			h.printVal(buf, formatAny(attr), output.Cyan)
 		}
 	case slog.KindString:
-		h.printVal(buf, formatString(attr), fgHiCyan)
+		h.printVal(buf, formatString(attr), output.Cyan)
 	default:
-		h.printVal(buf, formatAny(attr), fgHiBlue)
+		h.printVal(buf, formatAny(attr), output.Blue)
 	}
 }
 

internal/output/color.go

@@ -2,11 +2,22 @@ package output
 
 import "fmt"
 
-type ColorFn func(format string, a ...any) string
+type Color int
 
-func MaybeColor(fn ColorFn, disabled bool, format string, a ...any) string {
+const (
+	Dim     Color = 2
+	Black   Color = 90
+	Red     Color = 91
+	Yellow  Color = 93
+	Blue    Color = 94
+	Magenta Color = 95
+	Cyan    Color = 96
+	White   Color = 97
+)
+
+func MaybeColor(color Color, disabled bool, s string) string {
 	if disabled {
-		return fmt.Sprintf(format, a...)
+		return s
 	}
-	return fn(format, a...)
+	return fmt.Sprintf("\033[%dm%s\033[0m", color, s)
 }

internal/reporter/console.go

@@ -7,8 +7,6 @@ import (
 	"os"
 	"strings"
 
-	"github.com/fatih/color"
-
 	"github.com/cloudflare/pint/internal/checks"
 	"github.com/cloudflare/pint/internal/output"
 )
@@ -44,26 +42,28 @@ func (cr ConsoleReporter) Submit(summary Summary) (err error) {
 			}
 			buf.Reset()
 
-			buf.WriteString(output.MaybeColor(color.CyanString, cr.noColor, report.Path.Name)) // nolint:govet
+			buf.WriteString(output.MaybeColor(output.Cyan, cr.noColor, report.Path.Name))
 			if report.Path.Name != report.Path.SymlinkTarget {
-				buf.WriteString(output.MaybeColor(color.CyanString, cr.noColor, " ~> %s", report.Path.SymlinkTarget))
+				buf.WriteString(output.MaybeColor(output.Cyan, cr.noColor, " ~> "+report.Path.SymlinkTarget))
 			}
-			buf.WriteString(output.MaybeColor(color.CyanString, cr.noColor, ":%s", report.Problem.Lines.String()))
+			buf.WriteString(output.MaybeColor(output.Cyan, cr.noColor, ":"+report.Problem.Lines.String()))
 			if report.Problem.Anchor == checks.AnchorBefore {
-				buf.WriteRune(' ')
-				buf.WriteString(output.MaybeColor(color.RedString, cr.noColor, "(deleted)"))
+				buf.WriteString(output.MaybeColor(output.Red, cr.noColor, " (deleted)"))
 			}
 			buf.WriteRune(' ')
 
 			switch report.Problem.Severity {
 			case checks.Bug, checks.Fatal:
-				buf.WriteString(output.MaybeColor(color.RedString, cr.noColor, "%s: %s", report.Problem.Severity, report.Problem.Text))
+				buf.WriteString(output.MaybeColor(output.Red, cr.noColor,
+					report.Problem.Severity.String()+": "+report.Problem.Text))
 			case checks.Warning:
-				buf.WriteString(output.MaybeColor(color.YellowString, cr.noColor, "%s: %s", report.Problem.Severity, report.Problem.Text))
+				buf.WriteString(output.MaybeColor(output.Yellow, cr.noColor,
+					report.Problem.Severity.String()+": "+report.Problem.Text))
 			case checks.Information:
-				buf.WriteString(output.MaybeColor(color.HiBlackString, cr.noColor, "%s: %s", report.Problem.Severity, report.Problem.Text))
+				buf.WriteString(output.MaybeColor(output.Black, cr.noColor,
+					report.Problem.Severity.String()+": "+report.Problem.Text))
 			}
-			buf.WriteString(output.MaybeColor(color.MagentaString, cr.noColor, " (%s)\n", report.Problem.Reporter))
+			buf.WriteString(output.MaybeColor(output.Magenta, cr.noColor, " ("+report.Problem.Reporter+")\n"))
 
 			if report.Problem.Anchor == checks.AnchorAfter {
 				lines := strings.Split(content, "\n")
@@ -78,7 +78,7 @@ func (cr ConsoleReporter) Submit(summary Summary) (err error) {
 
 				nrFmt := fmt.Sprintf("%%%dd", countDigits(lastLine)+1)
 				for i := report.Problem.Lines.First; i <= lastLine; i++ {
-					buf.WriteString(output.MaybeColor(color.WhiteString, cr.noColor, nrFmt+" | %s\n", i, lines[i-1]))
+					buf.WriteString(output.MaybeColor(output.White, cr.noColor, fmt.Sprintf(nrFmt+" | %s\n", i, lines[i-1])))
 				}
 			}