diff --git a/internal/cli/format.go b/internal/cli/format.go index a6df44b..cfa254a 100644 --- a/internal/cli/format.go +++ b/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 - 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) - } - } + ctx = format.RegisterFormatters(ctx, cfg.Formatters) - 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 { - formatter := formatters[name] + for name := range cfg.Formatters { + 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 formatters { - if formatter.Parent() != nil { - // this formatter is not a root, it will be closed by a parent - continue - } + for _, formatter := range cfg.Formatters { formatter.Close() } - // await completion - for _, formatter := range formatters { - formatter.AwaitCompletion() + if count == 0 { + close(completedCh) } - // indicate no more completion events - close(completedCh) - return nil }) diff --git a/internal/cli/format_test.go b/internal/cli/format_test.go index cc89893..065b2d6 100644 --- a/internal/cli/format_test.go +++ b/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") -} diff --git a/internal/format/config.go b/internal/format/config.go index e404d52..29b127d 100644 --- a/internal/format/config.go +++ b/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. diff --git a/internal/format/context.go b/internal/format/context.go index d1fb703..7de916b 100644 --- a/internal/format/context.go +++ b/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 } diff --git a/internal/format/format.go b/internal/format/format.go index 76a9fb1..26f9a66 100644 --- a/internal/format/format.go +++ b/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. - inboxCh chan string - // completedCh is used to wait for this formatter to finish all processing. - completedCh chan interface{} + // inbox is used to accept new paths for formatting. + inbox chan string - // 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. -func NewFormatter(name string, config *FormatterConfig, globalExcludes []glob.Glob) (*Formatter, error) { +// Init is used to initialise internal state before this Formatter is ready to accept paths. +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.inboxCh = make(chan string, f.batchSize) - f.completedCh = make(chan interface{}, 1) + f.inbox = make(chan string, f.batchSize) + f.batch = make([]string, f.batchSize) + 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 -} - -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 + return nil } // 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: - // check if the inboxCh has been closed + case path, ok := <-f.inbox: + // 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 - for _, path := range f.batch { - MarkPathComplete(ctx, path) - } - } else { - // otherwise forward each path onto the next formatter for processing - for _, path := range f.batch { - f.child.Put(path) - } + // mark each path in this batch as completed + for _, path := range f.batch { + MarkFormatComplete(ctx, 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) -} - -func (f *Formatter) AwaitCompletion() { - // todo support a timeout - <-f.completedCh + close(f.inbox) }