diff --git a/internal/cli/format.go b/internal/cli/format.go index cfa254a..a6df44b 100644 --- a/internal/cli/format.go +++ b/internal/cli/format.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "os/signal" + "strings" "syscall" "time" @@ -64,8 +65,46 @@ 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, formatter := range cfg.Formatters { + for name, config := range cfg.Formatters { if !includeFormatter(name) { // remove this formatter delete(cfg.Formatters, name) @@ -73,24 +112,36 @@ func (f *Format) Run() error { continue } - err = formatter.Init(name, globalExcludes) + formatter, err := format.NewFormatter(name, config, globalExcludes) if errors.Is(err, format.ErrFormatterNotFound) && Cli.AllowMissingFormatter { l.Debugf("formatter not found: %v", name) - // remove this formatter - delete(cfg.Formatters, name) + continue } else if err != nil { return fmt.Errorf("%w: failed to initialise formatter: %v", err, name) } + + formatters[name] = formatter } - ctx = format.RegisterFormatters(ctx, cfg.Formatters) + // 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) + } + } - if err = cache.Open(Cli.TreeRoot, Cli.ClearCache, cfg.Formatters); err != nil { + if err = cache.Open(Cli.TreeRoot, Cli.ClearCache, formatters); err != nil { return err } // - pendingCh := make(chan string, 1024) completedCh := make(chan string, 1024) ctx = format.SetCompletedChannel(ctx, completedCh) @@ -99,8 +150,8 @@ func (f *Format) Run() error { eg, ctx := errgroup.WithContext(ctx) // start the formatters - for name := range cfg.Formatters { - formatter := cfg.Formatters[name] + for name := range formatters { + formatter := formatters[name] eg.Go(func() error { return formatter.Run(ctx) }) @@ -114,20 +165,13 @@ func (f *Format) Run() error { batchSize := 1024 batch := make([]string, 0, batchSize) - var pending, completed, changes int + var 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 @@ -141,12 +185,6 @@ func (f *Format) Run() error { changes += count batch = batch[:0] } - - completed += 1 - - if completed == pending { - close(completedCh) - } } } @@ -166,26 +204,32 @@ func (f *Format) Run() error { }) eg.Go(func() error { - count := 0 - + // pass paths to each formatter for path := range pathsCh { - for _, formatter := range cfg.Formatters { + for _, formatter := range formatters { if formatter.Wants(path) { - pendingCh <- path - count += 1 formatter.Put(path) } } } - for _, formatter := range cfg.Formatters { + // 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 + } formatter.Close() } - if count == 0 { - close(completedCh) + // await completion + for _, formatter := range formatters { + formatter.AwaitCompletion() } + // indicate no more completion events + close(completedCh) + return nil }) diff --git a/internal/cli/format_test.go b/internal/cli/format_test.go index 065b2d6..cc89893 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.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "foo-fmt": { Command: "foo-fmt", }, @@ -39,6 +39,27 @@ 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) @@ -46,7 +67,7 @@ func TestSpecifyingFormatters(t *testing.T) { configPath := tempDir + "/treefmt.toml" test.WriteConfig(t, configPath, format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "elm": { Command: "echo", Includes: []string{"*.elm"}, @@ -95,7 +116,7 @@ func TestIncludesAndExcludes(t *testing.T) { // test without any excludes config := format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "echo": { Command: "echo", Includes: []string{"*"}, @@ -167,7 +188,7 @@ func TestCache(t *testing.T) { // test without any excludes config := format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "echo": { Command: "echo", Includes: []string{"*"}, @@ -202,7 +223,7 @@ func TestChangeWorkingDirectory(t *testing.T) { // test without any excludes config := format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "echo": { Command: "echo", Includes: []string{"*"}, @@ -227,7 +248,7 @@ func TestFailOnChange(t *testing.T) { // test without any excludes config := format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "echo": { Command: "echo", Includes: []string{"*"}, @@ -263,7 +284,7 @@ func TestBustCacheOnFormatterChange(t *testing.T) { // start with 2 formatters config := format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "python": { Command: "black", Includes: []string{"*.py"}, @@ -307,7 +328,7 @@ func TestBustCacheOnFormatterChange(t *testing.T) { as.Contains(string(out), "0 files changed") // add go formatter - config.Formatters["go"] = &format.Formatter{ + config.Formatters["go"] = &format.FormatterConfig{ Command: "gofmt", Options: []string{"-w"}, Includes: []string{"*.go"}, @@ -358,7 +379,7 @@ func TestGitWorktree(t *testing.T) { // basic config config := format.Config{ - Formatters: map[string]*format.Formatter{ + Formatters: map[string]*format.FormatterConfig{ "echo": { Command: "echo", Includes: []string{"*"}, @@ -404,3 +425,57 @@ 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 29b127d..e404d52 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]*Formatter `toml:"formatter"` + Formatters map[string]*FormatterConfig `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 7de916b..d1fb703 100644 --- a/internal/format/context.go +++ b/internal/format/context.go @@ -5,28 +5,17 @@ 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) } -// MarkFormatComplete is used to indicate that all processing has finished for the provided path. +// MarkPathComplete 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 MarkFormatComplete(ctx context.Context, path string) { +func MarkPathComplete(ctx context.Context, path string) { ctx.Value(completedChKey).(chan string) <- path } diff --git a/internal/format/format.go b/internal/format/format.go index 26f9a66..76a9fb1 100644 --- a/internal/format/format.go +++ b/internal/format/format.go @@ -14,8 +14,7 @@ import ( // ErrFormatterNotFound is returned when the Command for a Formatter is not available. var ErrFormatterNotFound = errors.New("formatter not found") -// Formatter represents a command which should be applied to a filesystem. -type Formatter struct { +type FormatterConfig struct { // Command is the command invoke when applying this Formatter. Command string // Options are an optional list of args to be passed to Command. @@ -24,64 +23,100 @@ type Formatter 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 - // inbox is used to accept new paths for formatting. - inbox chan string + // 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{} - // Entries from inbox are batched according to batchSize and stored in batch for processing when the batchSize has + // Entries from inboxCh 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 } -// 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 { +// NewFormatter is used to create a new Formatter. +func NewFormatter(name string, config *FormatterConfig, globalExcludes []glob.Glob) (*Formatter, 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(f.Command) + executable, err := exec.LookPath(config.Command) if errors.Is(err, exec.ErrNotFound) { - return ErrFormatterNotFound + return nil, ErrFormatterNotFound } else if err != nil { - return err + return nil, err } f.executable = executable // initialise internal state f.log = log.WithPrefix("format | " + name) f.batchSize = 1024 - f.inbox = make(chan string, f.batchSize) - f.batch = make([]string, f.batchSize) - f.batch = f.batch[:0] + f.batch = make([]string, 0, f.batchSize) + f.inboxCh = make(chan string, f.batchSize) + f.completedCh = make(chan interface{}, 1) - f.includes, err = CompileGlobs(f.Includes) + f.includes, err = CompileGlobs(config.Includes) if err != nil { - return fmt.Errorf("%w: formatter '%v' includes", err, f.name) + return nil, fmt.Errorf("%w: formatter '%v' includes", err, f.name) } - f.excludes, err = CompileGlobs(f.Excludes) + f.excludes, err = CompileGlobs(config.Excludes) if err != nil { - return fmt.Errorf("%w: formatter '%v' excludes", err, f.name) + return nil, fmt.Errorf("%w: formatter '%v' excludes", err, f.name) } f.excludes = append(f.excludes, globalExcludes...) - return nil + 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 } // Wants is used to test if a Formatter wants path based on it's configured Includes and Excludes patterns. @@ -94,16 +129,26 @@ func (f *Formatter) Wants(path string) bool { return match } -// Put add path into this Formatter's inbox for processing. +// Put add path into this Formatter's inboxCh for processing. func (f *Formatter) Put(path string) { - f.inbox <- path + f.inboxCh <- 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 inbox has been closed + // keep processing until ctx has been cancelled or inboxCh has been closed for { select { @@ -112,8 +157,8 @@ LOOP: err = ctx.Err() break LOOP - case path, ok := <-f.inbox: - // check if the inbox has been closed + case path, ok := <-f.inboxCh: + // check if the inboxCh has been closed if !ok { break LOOP } @@ -148,7 +193,7 @@ func (f *Formatter) apply(ctx context.Context) error { } // construct args, starting with config - args := f.Options + args := f.config.Options // append each file path for _, path := range f.batch { @@ -157,7 +202,7 @@ func (f *Formatter) apply(ctx context.Context) error { // execute start := time.Now() - cmd := exec.CommandContext(ctx, f.Command, args...) + cmd := exec.CommandContext(ctx, f.config.Command, args...) if out, err := cmd.CombinedOutput(); err != nil { f.log.Debugf("\n%v", string(out)) @@ -167,9 +212,16 @@ func (f *Formatter) apply(ctx context.Context) error { f.log.Infof("%v files processed in %v", len(f.batch), time.Now().Sub(start)) - // mark each path in this batch as completed - for _, path := range f.batch { - MarkFormatComplete(ctx, path) + 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) + } } // reset batch @@ -180,5 +232,10 @@ 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.inbox) + close(f.inboxCh) +} + +func (f *Formatter) AwaitCompletion() { + // todo support a timeout + <-f.completedCh }