Merge pull request #1216 from cloudflare/console

Respect --no-color in console reporter
cloudflare · Dec 6, 2024 · e3c037c · e3c037c
2 parents 0c925ff + b1aaded
commit e3c037c
Show file tree

Hide file tree

Showing 7 changed files with 90 additions and 70 deletions.
diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go
@@ -137,7 +137,7 @@ func actionCI(c *cli.Context) error {
 	if c.Bool(teamCityFlag) {
 		reps = append(reps, reporter.NewTeamCityReporter(os.Stderr))
 	} else {
-		reps = append(reps, reporter.NewConsoleReporter(os.Stderr, checks.Information))
+		reps = append(reps, reporter.NewConsoleReporter(os.Stderr, checks.Information, c.Bool(noColorFlag)))
 	}
 	if c.String(checkStyleFlag) != "" {
 		var f *os.File

diff --git a/cmd/pint/lint.go b/cmd/pint/lint.go
@@ -119,7 +119,7 @@ func actionLint(c *cli.Context) error {
 	if c.Bool(teamCityFlag) {
 		reps = append(reps, reporter.NewTeamCityReporter(os.Stderr))
 	} else {
-		reps = append(reps, reporter.NewConsoleReporter(os.Stderr, minSeverity))
+		reps = append(reps, reporter.NewConsoleReporter(os.Stderr, minSeverity, c.Bool(noColorFlag)))
 	}
 
 	if c.String(checkStyleFlag) != "" {

diff --git a/docs/changelog.md b/docs/changelog.md
@@ -15,6 +15,10 @@
   }
   ```
 
+### Fixed
+
+- The console reporter won't color the output if `--no-color` flag is set.
+
 ## v0.68.0
 
 ### Added

diff --git a/internal/output/color.go b/internal/output/color.go
@@ -0,0 +1,12 @@
+package output
+
+import "fmt"
+
+type ColorFn func(format string, a ...any) string
+
+func MaybeColor(fn ColorFn, disabled bool, format string, a ...any) string {
+	if disabled {
+		return fmt.Sprintf(format, a...)
+	}
+	return fn(format, a...)
+}
diff --git a/internal/reporter/console.go b/internal/reporter/console.go
@@ -5,93 +5,84 @@ import (
 	"io"
 	"log/slog"
 	"os"
-	"slices"
 	"strings"
 
 	"github.com/fatih/color"
 
 	"github.com/cloudflare/pint/internal/checks"
+	"github.com/cloudflare/pint/internal/output"
 )
 
-func NewConsoleReporter(output io.Writer, minSeverity checks.Severity) ConsoleReporter {
-	return ConsoleReporter{output: output, minSeverity: minSeverity}
+func NewConsoleReporter(output io.Writer, minSeverity checks.Severity, noColor bool) ConsoleReporter {
+	return ConsoleReporter{
+		output:      output,
+		minSeverity: minSeverity,
+		noColor:     noColor,
+	}
 }
 
 type ConsoleReporter struct {
 	output      io.Writer
 	minSeverity checks.Severity
+	noColor     bool
 }
 
 func (cr ConsoleReporter) Submit(summary Summary) (err error) {
-	perFile := map[string][]string{}
-	for _, report := range summary.Reports() {
-		if report.Problem.Severity < cr.minSeverity {
-			continue
-		}
-
-		if _, ok := perFile[report.Path.Name]; !ok {
-			perFile[report.Path.Name] = []string{}
-		}
-
-		var content string
-		if report.Problem.Anchor == checks.AnchorAfter {
-			content, err = readFile(report.Path.Name)
-			if err != nil {
-				return err
+	var buf strings.Builder
+	var content string
+	for _, reports := range summary.ReportsPerPath() {
+		content = ""
+		for _, report := range reports {
+			if report.Problem.Severity < cr.minSeverity {
+				continue
 			}
-		}
-
-		path := report.Path.Name
-		if report.Path.Name != report.Path.SymlinkTarget {
-			path = fmt.Sprintf("%s ~> %s", report.Path.Name, report.Path.SymlinkTarget)
-		}
-		path = color.CyanString("%s:%s", path, report.Problem.Lines)
-		if report.Problem.Anchor == checks.AnchorBefore {
-			path += " " + color.RedString("(deleted)")
-		}
-		path += " "
-
-		msg := []string{path}
-		switch report.Problem.Severity {
-		case checks.Bug, checks.Fatal:
-			msg = append(msg, color.RedString("%s: %s", report.Problem.Severity, report.Problem.Text))
-		case checks.Warning:
-			msg = append(msg, color.YellowString("%s: %s", report.Problem.Severity, report.Problem.Text))
-		case checks.Information:
-			msg = append(msg, color.HiBlackString("%s: %s", report.Problem.Severity, report.Problem.Text))
-		}
-		msg = append(msg, color.MagentaString(" (%s)\n", report.Problem.Reporter))
-
-		if report.Problem.Anchor == checks.AnchorAfter {
-			lines := strings.Split(content, "\n")
-			lastLine := report.Problem.Lines.Last
-			if lastLine > len(lines)-1 {
-				lastLine = len(lines) - 1
-				slog.Warn(
-					"Tried to read more lines than present in the source file, this is likely due to '\n' usage in some rules, see https://github.com/cloudflare/pint/issues/20 for details",
-					slog.String("path", report.Path.Name),
-				)
+			if content == "" && report.Problem.Anchor == checks.AnchorAfter {
+				content, err = readFile(report.Path.Name)
+				if err != nil {
+					return err
+				}
 			}
+			buf.Reset()
 
-			nrFmt := fmt.Sprintf("%%%dd", countDigits(lastLine)+1)
-			for i := report.Problem.Lines.First; i <= lastLine; i++ {
-				msg = append(msg, color.WhiteString(nrFmt+" | %s\n", i, lines[i-1]))
+			buf.WriteString(output.MaybeColor(color.CyanString, cr.noColor, report.Path.Name)) // nolint:govet
+			if report.Path.Name != report.Path.SymlinkTarget {
+				buf.WriteString(output.MaybeColor(color.CyanString, cr.noColor, " ~> %s", report.Path.SymlinkTarget))
+			}
+			buf.WriteString(output.MaybeColor(color.CyanString, cr.noColor, ":%s", report.Problem.Lines.String()))
+			if report.Problem.Anchor == checks.AnchorBefore {
+				buf.WriteRune(' ')
+				buf.WriteString(output.MaybeColor(color.RedString, 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))
+			case checks.Warning:
+				buf.WriteString(output.MaybeColor(color.YellowString, cr.noColor, "%s: %s", report.Problem.Severity, 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(color.MagentaString, cr.noColor, " (%s)\n", report.Problem.Reporter))
+
+			if report.Problem.Anchor == checks.AnchorAfter {
+				lines := strings.Split(content, "\n")
+				lastLine := report.Problem.Lines.Last
+				if lastLine > len(lines)-1 {
+					lastLine = len(lines) - 1
+					slog.Warn(
+						"Tried to read more lines than present in the source file, this is likely due to '\n' usage in some rules, see https://github.com/cloudflare/pint/issues/20 for details",
+						slog.String("path", report.Path.Name),
+					)
+				}
+
+				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]))
+				}
 			}
-		}
-
-		perFile[report.Path.Name] = append(perFile[report.Path.Name], strings.Join(msg, ""))
-	}
-
-	paths := []string{}
-	for path := range perFile {
-		paths = append(paths, path)
-	}
-	slices.Sort(paths)
 
-	for _, path := range paths {
-		msgs := perFile[path]
-		for _, msg := range msgs {
-			fmt.Fprintln(cr.output, msg)
+			fmt.Fprintln(cr.output, buf.String())
 		}
 	}
 

diff --git a/internal/reporter/github.go b/internal/reporter/github.go
@@ -367,7 +367,7 @@ func formatGHReviewBody(version string, summary Summary) string {
 	b.WriteString("<details><summary>Problems</summary>\n<p>\n\n")
 	if len(summary.Reports()) > 0 {
 		buf := bytes.NewBuffer(nil)
-		cr := NewConsoleReporter(buf, checks.Information)
+		cr := NewConsoleReporter(buf, checks.Information, true)
 		err := cr.Submit(summary)
 		if err != nil {
 			b.WriteString(fmt.Sprintf("Failed to generate list of problems: %s", err))

diff --git a/internal/reporter/reporter.go b/internal/reporter/reporter.go
@@ -96,6 +96,19 @@ func (s Summary) Reports() (reports []Report) {
 	return s.reports
 }
 
+func (s Summary) ReportsPerPath() (reports [][]Report) {
+	curPath := []Report{}
+	for _, r := range s.reports {
+		if len(curPath) > 0 && curPath[0].Path.Name != r.Path.Name {
+			reports = append(reports, curPath)
+			curPath = []Report{}
+		}
+		curPath = append(curPath, r)
+	}
+	reports = append(reports, curPath)
+	return reports
+}
+
 func (s Summary) HasFatalProblems() bool {
 	for _, r := range s.Reports() {
 		if r.Problem.Severity == checks.Fatal {