Merge pull request #453 from numtide/fix/exit-with-error-formatters-fail

fix: exit with error if any formatters fail

Author: brianmcgee

cmd/format/format.go

@@ -32,7 +32,10 @@ const (
 	BatchSize = 1024
 )
 
-var ErrFailOnChange = errors.New("unexpected changes detected, --fail-on-change is enabled")
+var (
+	ErrFailOnChange       = errors.New("unexpected changes detected, --fail-on-change is enabled")
+	ErrFormattingFailures = errors.New("formatting failures detected")
+)
 
 func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string) error {
 	cmd.SilenceUsage = true
@@ -404,6 +407,8 @@ func postProcessing(
 	formattedCh chan *format.Task,
 ) func() error {
 	return func() error {
+		var formattingFailures bool // track if there were any formatting failures
+
 	LOOP:
 		for {
 			select {
@@ -420,8 +425,9 @@ func postProcessing(
 				// grab the underlying file reference
 				file := task.File
 
-				// check if there were any errors processing the file
 				if len(task.Errors) > 0 {
+					formattingFailures = true
+
 					// release the file, passing the first task error
 					// note: task errors are related to the batch in which a task was applied
 					// this does not necessarily indicate this file had a problem being formatted, but this approach
@@ -471,16 +477,21 @@ func postProcessing(
 			}
 		}
 
-		// if fail on change has been enabled, check that no files were actually changed, throwing an error if so
-		if cfg.FailOnChange && statz.Value(stats.Changed) != 0 {
-			return ErrFailOnChange
-		}
-
 		// print stats to stdout unless we are processing stdin and printing the results to stdout
 		if !cfg.Stdin {
 			statz.Print()
 		}
 
+		// return an error if any formatting failures were detected
+		if formattingFailures {
+			return ErrFormattingFailures
+		}
+
+		// if fail on change has been enabled, check that no files were actually changed, throwing an error if so
+		if cfg.FailOnChange && statz.Value(stats.Changed) != 0 {
+			return ErrFailOnChange
+		}
+
 		return nil
 	}
 }

cmd/root_test.go

@@ -9,12 +9,13 @@ import (
 	"path"
 	"path/filepath"
 	"regexp"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/charmbracelet/log"
 	"github.com/numtide/treefmt/cmd"
-	format2 "github.com/numtide/treefmt/cmd/format"
+	formatCmd "github.com/numtide/treefmt/cmd/format"
 	"github.com/numtide/treefmt/config"
 	"github.com/numtide/treefmt/format"
 	"github.com/numtide/treefmt/stats"
@@ -482,7 +483,7 @@ func TestCache(t *testing.T) {
 
 	// running should match but not format anything
 	_, statz, err = treefmt(t, "--config-file", configPath, "--tree-root", tempDir)
-	as.NoError(err)
+	as.ErrorIs(err, formatCmd.ErrFormattingFailures)
 
 	assertStats(t, as, statz, map[stats.Type]int{
 		stats.Traversed: 32,
@@ -492,8 +493,8 @@ func TestCache(t *testing.T) {
 	})
 
 	// running again should provide the same result
-	_, statz, err = treefmt(t, "--config-file", configPath, "--tree-root", tempDir, "-vv")
-	as.NoError(err)
+	_, statz, err = treefmt(t, "--config-file", configPath, "--tree-root", tempDir)
+	as.ErrorIs(err, formatCmd.ErrFormattingFailures)
 
 	assertStats(t, as, statz, map[stats.Type]int{
 		stats.Traversed: 32,
@@ -588,13 +589,13 @@ func TestFailOnChange(t *testing.T) {
 
 	test.WriteConfig(t, configPath, cfg)
 	_, _, err := treefmt(t, "--fail-on-change", "--config-file", configPath, "--tree-root", tempDir)
-	as.ErrorIs(err, format2.ErrFailOnChange)
+	as.ErrorIs(err, formatCmd.ErrFailOnChange)
 
 	// test with no cache
 	t.Setenv("TREEFMT_FAIL_ON_CHANGE", "true")
 	test.WriteConfig(t, configPath, cfg)
 	_, _, err = treefmt(t, "--config-file", configPath, "--tree-root", tempDir, "--no-cache")
-	as.ErrorIs(err, format2.ErrFailOnChange)
+	as.ErrorIs(err, formatCmd.ErrFailOnChange)
 }
 
 func TestBustCacheOnFormatterChange(t *testing.T) {
@@ -1027,7 +1028,7 @@ func TestStdin(t *testing.T) {
 	// we get an error about the missing filename parameter.
 	out, _, err := treefmt(t, "-C", tempDir, "--allow-missing-formatter", "--stdin")
 	as.EqualError(err, "exactly one path should be specified when using the --stdin flag")
-	as.Equal("", string(out))
+	as.Equal("Error: exactly one path should be specified when using the --stdin flag\n", string(out))
 
 	// now pass along the filename parameter
 	os.Stdin = test.TempFile(t, "", "stdin", &contents)
@@ -1051,7 +1052,7 @@ func TestStdin(t *testing.T) {
 
 	out, _, err = treefmt(t, "-C", tempDir, "--allow-missing-formatter", "--stdin", "../test.nix")
 	as.Errorf(err, "path ../test.nix not inside the tree root %s", tempDir)
-	as.Equal("", string(out))
+	as.Contains(string(out), "Error: path ../test.nix not inside the tree root")
 
 	// try some markdown instead
 	contents = `
@@ -1239,6 +1240,8 @@ func TestRunInSubdir(t *testing.T) {
 func treefmt(t *testing.T, args ...string) ([]byte, *stats.Stats, error) {
 	t.Helper()
 
+	t.Logf("treefmt %s", strings.Join(args, " "))
+
 	tempDir := t.TempDir()
 	tempOut := test.TempFile(t, tempDir, "combined_output", nil)
 
@@ -1281,21 +1284,22 @@ func treefmt(t *testing.T, args ...string) ([]byte, *stats.Stats, error) {
 		time.Sleep(time.Until(waitUntil))
 	}()
 
-	if err := root.Execute(); err != nil {
-		return nil, nil, err
-	}
+	// execute the command
+	cmdErr := root.Execute()
 
 	// reset and read the temporary output
-	if _, err := tempOut.Seek(0, 0); err != nil {
-		return nil, nil, fmt.Errorf("failed to reset temp output for reading: %w", err)
+	if _, resetErr := tempOut.Seek(0, 0); resetErr != nil {
+		t.Fatal(fmt.Errorf("failed to reset temp output for reading: %w", resetErr))
 	}
 
-	out, err := io.ReadAll(tempOut)
-	if err != nil {
-		return nil, nil, fmt.Errorf("failed to read temp output: %w", err)
+	out, readErr := io.ReadAll(tempOut)
+	if readErr != nil {
+		t.Fatal(fmt.Errorf("failed to read temp output: %w", readErr))
 	}
 
-	return out, statz, nil
+	t.Log(string(out))
+
+	return out, statz, cmdErr
 }
 
 func assertStats(

format/formatter.go

@@ -77,7 +77,7 @@ func (f *Formatter) Apply(ctx context.Context, tasks []*Task) error {
 		f.log.Errorf("failed to apply with options '%v': %s", f.config.Options, err)
 
 		if len(out) > 0 {
-			_, _ = fmt.Fprintf(os.Stderr, "%s error:\n%s\n", f.name, out)
+			_, _ = fmt.Fprintf(os.Stderr, "\n%s\n", out)
 		}
 
 		return fmt.Errorf("formatter '%s' with options '%v' failed to apply: %w", f.config.Command, f.config.Options, err)