Revert "support formatter ordering (#20)"

This reverts commit c7d0138a02.

internal/cli/format.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
-	"strings"
 	"syscall"
 	"time"
 
@@ -65,46 +64,8 @@ func (f *Format) Run() error {
 		}
 	}
 
-	formatters := make(map[string]*format.Formatter)
-
-	// detect broken dependencies
-	for name, config := range cfg.Formatters {
-		before := config.Before
-		if before != "" {
-			// check child formatter exists
-			_, ok := cfg.Formatters[before]
-			if !ok {
-				return fmt.Errorf("formatter %v is before %v but config for %v was not found", name, before, before)
-			}
-		}
-	}
-
-	// dependency cycle detection
-	for name, config := range cfg.Formatters {
-		var ok bool
-		var history []string
-		childName := name
-		for {
-			// add to history
-			history = append(history, childName)
-
-			if config.Before == "" {
-				break
-			} else if config.Before == name {
-				return fmt.Errorf("formatter cycle detected %v", strings.Join(history, " -> "))
-			}
-
-			// load child config
-			childName = config.Before
-			config, ok = cfg.Formatters[config.Before]
-			if !ok {
-				return fmt.Errorf("formatter not found: %v", config.Before)
-			}
-		}
-	}
-
 	// init formatters
-	for name, config := range cfg.Formatters {
+	for name, formatter := range cfg.Formatters {
 		if !includeFormatter(name) {
 			// remove this formatter
 			delete(cfg.Formatters, name)
@@ -112,36 +73,24 @@ func (f *Format) Run() error {
 			continue
 		}
 
-		formatter, err := format.NewFormatter(name, config, globalExcludes)
+		err = formatter.Init(name, globalExcludes)
 		if errors.Is(err, format.ErrFormatterNotFound) && Cli.AllowMissingFormatter {
 			l.Debugf("formatter not found: %v", name)
-			continue
+			// remove this formatter
+			delete(cfg.Formatters, name)
 		} else if err != nil {
 			return fmt.Errorf("%w: failed to initialise formatter: %v", err, name)
 		}
-
-		formatters[name] = formatter
 	}
 
-	// iterate the initialised formatters configuring parent/child relationships
+	ctx = format.RegisterFormatters(ctx, cfg.Formatters)
-	for _, formatter := range formatters {
-		if formatter.Before() != "" {
-			child, ok := formatters[formatter.Before()]
-			if !ok {
-				// formatter has been filtered out by the user
-				formatter.ResetBefore()
-				continue
-			}
-			formatter.SetChild(child)
-			child.SetParent(formatter)
-		}
-	}
 
-	if err = cache.Open(Cli.TreeRoot, Cli.ClearCache, formatters); err != nil {
+	if err = cache.Open(Cli.TreeRoot, Cli.ClearCache, cfg.Formatters); err != nil {
 		return err
 	}
 
 	//
+	pendingCh := make(chan string, 1024)
 	completedCh := make(chan string, 1024)
 
 	ctx = format.SetCompletedChannel(ctx, completedCh)
@@ -150,8 +99,8 @@ func (f *Format) Run() error {
 	eg, ctx := errgroup.WithContext(ctx)
 
 	// start the formatters
-	for name := range formatters {
+	for name := range cfg.Formatters {
-		formatter := formatters[name]
+		formatter := cfg.Formatters[name]
 		eg.Go(func() error {
 			return formatter.Run(ctx)
 		})
@@ -165,13 +114,20 @@ func (f *Format) Run() error {
 		batchSize := 1024
 		batch := make([]string, 0, batchSize)
 
-		var changes int
+		var pending, completed, changes int
 
 	LOOP:
 		for {
 			select {
 			case <-ctx.Done():
 				return ctx.Err()
+			case _, ok := <-pendingCh:
+				if ok {
+					pending += 1
+				} else if pending == completed {
+					break LOOP
+				}
+
 			case path, ok := <-completedCh:
 				if !ok {
 					break LOOP
@@ -185,6 +141,12 @@ func (f *Format) Run() error {
 					changes += count
 					batch = batch[:0]
 				}
+
+				completed += 1
+
+				if completed == pending {
+					close(completedCh)
+				}
 			}
 		}
 
@@ -204,32 +166,26 @@ func (f *Format) Run() error {
 	})
 
 	eg.Go(func() error {
-		// pass paths to each formatter
+		count := 0
+
 		for path := range pathsCh {
-			for _, formatter := range formatters {
+			for _, formatter := range cfg.Formatters {
 				if formatter.Wants(path) {
+					pendingCh <- path
+					count += 1
 					formatter.Put(path)
 				}
 			}
 		}
 
-		// indicate no more paths for each formatter
+		for _, formatter := range cfg.Formatters {
-		for _, formatter := range formatters {
-			if formatter.Parent() != nil {
-				// this formatter is not a root, it will be closed by a parent
-				continue
-			}
 			formatter.Close()
 		}
 
-		// await completion
+		if count == 0 {
-		for _, formatter := range formatters {
+			close(completedCh)
-			formatter.AwaitCompletion()
 		}
 
-		// indicate no more completion events
-		close(completedCh)
-
 		return nil
 	})
 

internal/cli/format_test.go

@@ -25,7 +25,7 @@ func TestAllowMissingFormatter(t *testing.T) {
 	configPath := tempDir + "/treefmt.toml"
 
 	test.WriteConfig(t, configPath, format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"foo-fmt": {
 				Command: "foo-fmt",
 			},
@@ -39,27 +39,6 @@ func TestAllowMissingFormatter(t *testing.T) {
 	as.NoError(err)
 }
 
-func TestDependencyCycle(t *testing.T) {
-	as := require.New(t)
-
-	tempDir := t.TempDir()
-	configPath := tempDir + "/treefmt.toml"
-
-	test.WriteConfig(t, configPath, format.Config{
-		Formatters: map[string]*format.FormatterConfig{
-			"a": {Command: "echo", Before: "b"},
-			"b": {Command: "echo", Before: "c"},
-			"c": {Command: "echo", Before: "a"},
-			"d": {Command: "echo", Before: "e"},
-			"e": {Command: "echo", Before: "f"},
-			"f": {Command: "echo"},
-		},
-	})
-
-	_, err := cmd(t, "--config-file", configPath, "--tree-root", tempDir)
-	as.ErrorContains(err, "formatter cycle detected a -> b -> c")
-}
-
 func TestSpecifyingFormatters(t *testing.T) {
 	as := require.New(t)
 
@@ -67,7 +46,7 @@ func TestSpecifyingFormatters(t *testing.T) {
 	configPath := tempDir + "/treefmt.toml"
 
 	test.WriteConfig(t, configPath, format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"elm": {
 				Command:  "echo",
 				Includes: []string{"*.elm"},
@@ -116,7 +95,7 @@ func TestIncludesAndExcludes(t *testing.T) {
 
 	// test without any excludes
 	config := format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"echo": {
 				Command:  "echo",
 				Includes: []string{"*"},
@@ -188,7 +167,7 @@ func TestCache(t *testing.T) {
 
 	// test without any excludes
 	config := format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"echo": {
 				Command:  "echo",
 				Includes: []string{"*"},
@@ -223,7 +202,7 @@ func TestChangeWorkingDirectory(t *testing.T) {
 
 	// test without any excludes
 	config := format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"echo": {
 				Command:  "echo",
 				Includes: []string{"*"},
@@ -248,7 +227,7 @@ func TestFailOnChange(t *testing.T) {
 
 	// test without any excludes
 	config := format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"echo": {
 				Command:  "echo",
 				Includes: []string{"*"},
@@ -284,7 +263,7 @@ func TestBustCacheOnFormatterChange(t *testing.T) {
 
 	// start with 2 formatters
 	config := format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"python": {
 				Command:  "black",
 				Includes: []string{"*.py"},
@@ -328,7 +307,7 @@ func TestBustCacheOnFormatterChange(t *testing.T) {
 	as.Contains(string(out), "0 files changed")
 
 	// add go formatter
-	config.Formatters["go"] = &format.FormatterConfig{
+	config.Formatters["go"] = &format.Formatter{
 		Command:  "gofmt",
 		Options:  []string{"-w"},
 		Includes: []string{"*.go"},
@@ -379,7 +358,7 @@ func TestGitWorktree(t *testing.T) {
 
 	// basic config
 	config := format.Config{
-		Formatters: map[string]*format.FormatterConfig{
+		Formatters: map[string]*format.Formatter{
 			"echo": {
 				Command:  "echo",
 				Includes: []string{"*"},
@@ -425,57 +404,3 @@ func TestGitWorktree(t *testing.T) {
 	as.NoError(err)
 	as.Contains(string(out), fmt.Sprintf("%d files changed", 55))
 }
-
-func TestOrderingFormatters(t *testing.T) {
-	as := require.New(t)
-
-	tempDir := test.TempExamples(t)
-	configPath := path.Join(tempDir, "treefmt.toml")
-
-	// missing child
-	test.WriteConfig(t, configPath, format.Config{
-		Formatters: map[string]*format.FormatterConfig{
-			"hs-a": {
-				Command:  "echo",
-				Includes: []string{"*.hs"},
-				Before:   "hs-b",
-			},
-		},
-	})
-
-	out, err := cmd(t, "--config-file", configPath, "--tree-root", tempDir)
-	as.ErrorContains(err, "formatter hs-a is before hs-b but config for hs-b was not found")
-
-	// multiple roots
-	test.WriteConfig(t, configPath, format.Config{
-		Formatters: map[string]*format.FormatterConfig{
-			"hs-a": {
-				Command:  "echo",
-				Includes: []string{"*.hs"},
-				Before:   "hs-b",
-			},
-			"hs-b": {
-				Command:  "echo",
-				Includes: []string{"*.hs"},
-				Before:   "hs-c",
-			},
-			"hs-c": {
-				Command:  "echo",
-				Includes: []string{"*.hs"},
-			},
-			"py-a": {
-				Command:  "echo",
-				Includes: []string{"*.py"},
-				Before:   "py-b",
-			},
-			"py-b": {
-				Command:  "echo",
-				Includes: []string{"*.py"},
-			},
-		},
-	})
-
-	out, err = cmd(t, "--config-file", configPath, "--tree-root", tempDir)
-	as.NoError(err)
-	as.Contains(string(out), "8 files changed")
-}

internal/format/config.go

@@ -8,7 +8,7 @@ type Config struct {
 		// Excludes is an optional list of glob patterns used to exclude certain files from all formatters.
 		Excludes []string
 	}
-	Formatters map[string]*FormatterConfig `toml:"formatter"`
+	Formatters map[string]*Formatter `toml:"formatter"`
 }
 
 // ReadConfigFile reads from path and unmarshals toml into a Config instance.

internal/format/context.go

@@ -5,17 +5,28 @@ import (
 )
 
 const (
+	formattersKey  = "formatters"
 	completedChKey = "completedCh"
 )
 
+// RegisterFormatters is used to set a map of formatters in the provided context.
+func RegisterFormatters(ctx context.Context, formatters map[string]*Formatter) context.Context {
+	return context.WithValue(ctx, formattersKey, formatters)
+}
+
+// GetFormatters is used to retrieve a formatters map from the provided context.
+func GetFormatters(ctx context.Context) map[string]*Formatter {
+	return ctx.Value(formattersKey).(map[string]*Formatter)
+}
+
 // SetCompletedChannel is used to set a channel for indication processing completion in the provided context.
 func SetCompletedChannel(ctx context.Context, completedCh chan string) context.Context {
 	return context.WithValue(ctx, completedChKey, completedCh)
 }
 
-// MarkPathComplete is used to indicate that all processing has finished for the provided path.
+// MarkFormatComplete is used to indicate that all processing has finished for the provided path.
 // This is done by adding the path to the completion channel which should have already been set using
 // SetCompletedChannel.
-func MarkPathComplete(ctx context.Context, path string) {
+func MarkFormatComplete(ctx context.Context, path string) {
 	ctx.Value(completedChKey).(chan string) <- path
 }

internal/format/format.go

@@ -14,7 +14,8 @@ import (
 // ErrFormatterNotFound is returned when the Command for a Formatter is not available.
 var ErrFormatterNotFound = errors.New("formatter not found")
 
-type FormatterConfig struct {
+// Formatter represents a command which should be applied to a filesystem.
+type Formatter struct {
 	// Command is the command invoke when applying this Formatter.
 	Command string
 	// Options are an optional list of args to be passed to Command.
@@ -23,100 +24,64 @@ type FormatterConfig struct {
 	Includes []string
 	// Excludes is an optional list of glob patterns used to exclude certain files from this Formatter.
 	Excludes []string
-	// Before is the name of another formatter which must process a path after this one
-	Before string
-}
-
-// Formatter represents a command which should be applied to a filesystem.
-type Formatter struct {
-	name   string
-	config *FormatterConfig
 
+	name       string
 	log        *log.Logger
 	executable string // path to the executable described by Command
 
-	before string
-	parent *Formatter
-	child  *Formatter
-
 	// internal compiled versions of Includes and Excludes.
 	includes []glob.Glob
 	excludes []glob.Glob
 
-	// inboxCh is used to accept new paths for formatting.
+	// inbox is used to accept new paths for formatting.
-	inboxCh chan string
+	inbox chan string
-	// completedCh is used to wait for this formatter to finish all processing.
-	completedCh chan interface{}
 
-	// Entries from inboxCh are batched according to batchSize and stored in batch for processing when the batchSize has
+	// Entries from inbox are batched according to batchSize and stored in batch for processing when the batchSize has
 	// been reached or Close is invoked.
 	batch     []string
 	batchSize int
 }
 
-func (f *Formatter) Before() string {
-	return f.before
-}
-
-func (f *Formatter) ResetBefore() {
-	f.before = ""
-}
-
 // Executable returns the path to the executable defined by Command
 func (f *Formatter) Executable() string {
 	return f.executable
 }
 
-// NewFormatter is used to create a new Formatter.
+// Init is used to initialise internal state before this Formatter is ready to accept paths.
-func NewFormatter(name string, config *FormatterConfig, globalExcludes []glob.Glob) (*Formatter, error) {
+func (f *Formatter) Init(name string, globalExcludes []glob.Glob) error {
 	var err error
 
-	f := Formatter{}
 	// capture the name from the config file
 	f.name = name
-	f.config = config
-	f.before = config.Before
 
 	// test if the formatter is available
-	executable, err := exec.LookPath(config.Command)
+	executable, err := exec.LookPath(f.Command)
 	if errors.Is(err, exec.ErrNotFound) {
-		return nil, ErrFormatterNotFound
+		return ErrFormatterNotFound
 	} else if err != nil {
-		return nil, err
+		return err
 	}
 	f.executable = executable
 
 	// initialise internal state
 	f.log = log.WithPrefix("format | " + name)
 	f.batchSize = 1024
-	f.batch = make([]string, 0, f.batchSize)
+	f.inbox = make(chan string, f.batchSize)
-	f.inboxCh = make(chan string, f.batchSize)
+	f.batch = make([]string, f.batchSize)
-	f.completedCh = make(chan interface{}, 1)
+	f.batch = f.batch[:0]
 
-	f.includes, err = CompileGlobs(config.Includes)
+	f.includes, err = CompileGlobs(f.Includes)
 	if err != nil {
-		return nil, fmt.Errorf("%w: formatter '%v' includes", err, f.name)
+		return fmt.Errorf("%w: formatter '%v' includes", err, f.name)
 	}
 
-	f.excludes, err = CompileGlobs(config.Excludes)
+	f.excludes, err = CompileGlobs(f.Excludes)
 	if err != nil {
-		return nil, fmt.Errorf("%w: formatter '%v' excludes", err, f.name)
+		return fmt.Errorf("%w: formatter '%v' excludes", err, f.name)
 	}
 	f.excludes = append(f.excludes, globalExcludes...)
 
-	return &f, nil
+	return nil
-}
-
-func (f *Formatter) SetParent(formatter *Formatter) {
-	f.parent = formatter
-}
-
-func (f *Formatter) Parent() *Formatter {
-	return f.parent
-}
-
-func (f *Formatter) SetChild(formatter *Formatter) {
-	f.child = formatter
 }
 
 // Wants is used to test if a Formatter wants path based on it's configured Includes and Excludes patterns.
@@ -129,26 +94,16 @@ func (f *Formatter) Wants(path string) bool {
 	return match
 }
 
-// Put add path into this Formatter's inboxCh for processing.
+// Put add path into this Formatter's inbox for processing.
 func (f *Formatter) Put(path string) {
-	f.inboxCh <- path
+	f.inbox <- path
 }
 
 // Run is the main processing loop for this Formatter.
 // It accepts a context which is used to lookup certain dependencies and for cancellation.
 func (f *Formatter) Run(ctx context.Context) (err error) {
-	defer func() {
-		if f.child != nil {
-			// indicate no further processing for the child formatter
-			f.child.Close()
-		}
-
-		// indicate this formatter has finished processing
-		f.completedCh <- nil
-	}()
-
 LOOP:
-	// keep processing until ctx has been cancelled or inboxCh has been closed
+	// keep processing until ctx has been cancelled or inbox has been closed
 	for {
 		select {
 
@@ -157,8 +112,8 @@ LOOP:
 			err = ctx.Err()
 			break LOOP
 
-		case path, ok := <-f.inboxCh:
+		case path, ok := <-f.inbox:
-			// check if the inboxCh has been closed
+			// check if the inbox has been closed
 			if !ok {
 				break LOOP
 			}
@@ -193,7 +148,7 @@ func (f *Formatter) apply(ctx context.Context) error {
 	}
 
 	// construct args, starting with config
-	args := f.config.Options
+	args := f.Options
 
 	// append each file path
 	for _, path := range f.batch {
@@ -202,7 +157,7 @@ func (f *Formatter) apply(ctx context.Context) error {
 
 	// execute
 	start := time.Now()
-	cmd := exec.CommandContext(ctx, f.config.Command, args...)
+	cmd := exec.CommandContext(ctx, f.Command, args...)
 
 	if out, err := cmd.CombinedOutput(); err != nil {
 		f.log.Debugf("\n%v", string(out))
@@ -212,16 +167,9 @@ func (f *Formatter) apply(ctx context.Context) error {
 
 	f.log.Infof("%v files processed in %v", len(f.batch), time.Now().Sub(start))
 
-	if f.child == nil {
+	// mark each path in this batch as completed
-		// mark each path in this batch as completed
+	for _, path := range f.batch {
-		for _, path := range f.batch {
+		MarkFormatComplete(ctx, path)
-			MarkPathComplete(ctx, path)
-		}
-	} else {
-		// otherwise forward each path onto the next formatter for processing
-		for _, path := range f.batch {
-			f.child.Put(path)
-		}
 	}
 
 	// reset batch
@@ -232,10 +180,5 @@ func (f *Formatter) apply(ctx context.Context) error {
 
 // Close is used to indicate that a Formatter should process any remaining paths and then stop it's processing loop.
 func (f *Formatter) Close() {
-	close(f.inboxCh)
+	close(f.inbox)
-}
-
-func (f *Formatter) AwaitCompletion() {
-	// todo support a timeout
-	<-f.completedCh
 }