support formatter ordering #20
|
@ -64,22 +64,23 @@ func (f *Format) Run() error {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
formatters := make(map[string]*format.Formatter)
|
||||||
|
|
||||||
// detect dependency cycles and broken dependencies
|
// detect dependency cycles and broken dependencies
|
||||||
for name, formatter := range cfg.Formatters {
|
// todo dependency cycle detection
|
||||||
if formatter.Before != "" {
|
for name, config := range cfg.Formatters {
|
||||||
|
before := config.Before
|
||||||
|
if before != "" {
|
||||||
// check child formatter exists
|
// check child formatter exists
|
||||||
_, ok := cfg.Formatters[formatter.Before]
|
_, ok := cfg.Formatters[before]
|
||||||
brianmcgee marked this conversation as resolved
Outdated
|
|||||||
if !ok {
|
if !ok {
|
||||||
return fmt.Errorf(
|
return fmt.Errorf("formatter %v is before %v but config for %v was not found", name, before, before)
|
||||||
brianmcgee marked this conversation as resolved
Outdated
zimbatm
commented
You need a second for loop here to check for the dependency cycle. You need a second for loop here to check for the dependency cycle.
brianmcgee
commented
Yeah I haven't done the dependency cycle check yet Yeah I haven't done the dependency cycle check yet
brianmcgee
commented
Addressed in Addressed in 23c86cee027e133181577f749f7716e1ad0986cb
|
|||||||
"formatter %v is before %v but config for %v was not found",
|
|
||||||
name, formatter.Before, formatter.Before,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// init formatters
|
// init formatters
|
||||||
for name, formatter := range cfg.Formatters {
|
for name, config := range cfg.Formatters {
|
||||||
if !includeFormatter(name) {
|
if !includeFormatter(name) {
|
||||||
// remove this formatter
|
// remove this formatter
|
||||||
delete(cfg.Formatters, name)
|
delete(cfg.Formatters, name)
|
||||||
|
@ -87,23 +88,24 @@ func (f *Format) Run() error {
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
err = formatter.Init(name, globalExcludes)
|
formatter, err := format.NewFormatter(name, config, globalExcludes)
|
||||||
if errors.Is(err, format.ErrFormatterNotFound) && Cli.AllowMissingFormatter {
|
if errors.Is(err, format.ErrFormatterNotFound) && Cli.AllowMissingFormatter {
|
||||||
l.Debugf("formatter not found: %v", name)
|
l.Debugf("formatter not found: %v", name)
|
||||||
// remove this formatter
|
continue
|
||||||
delete(cfg.Formatters, name)
|
|
||||||
} else if err != nil {
|
} else if err != nil {
|
||||||
return fmt.Errorf("%w: failed to initialise formatter: %v", err, name)
|
return fmt.Errorf("%w: failed to initialise formatter: %v", err, name)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
formatters[name] = formatter
|
||||||
}
|
}
|
||||||
|
|
||||||
// iterate the initialised formatters configuring parent/child relationships
|
// iterate the initialised formatters configuring parent/child relationships
|
||||||
for _, formatter := range cfg.Formatters {
|
for _, formatter := range formatters {
|
||||||
if formatter.Before != "" {
|
if formatter.Before() != "" {
|
||||||
child, ok := cfg.Formatters[formatter.Before]
|
child, ok := formatters[formatter.Before()]
|
||||||
if !ok {
|
if !ok {
|
||||||
// formatter has been filtered out by the user
|
// formatter has been filtered out by the user
|
||||||
formatter.Before = ""
|
formatter.ResetBefore()
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
formatter.SetChild(child)
|
formatter.SetChild(child)
|
||||||
|
@ -111,7 +113,7 @@ func (f *Format) Run() error {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
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
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -124,8 +126,8 @@ func (f *Format) Run() error {
|
||||||
eg, ctx := errgroup.WithContext(ctx)
|
eg, ctx := errgroup.WithContext(ctx)
|
||||||
|
|
||||||
// start the formatters
|
// start the formatters
|
||||||
for name := range cfg.Formatters {
|
for name := range formatters {
|
||||||
formatter := cfg.Formatters[name]
|
formatter := formatters[name]
|
||||||
eg.Go(func() error {
|
eg.Go(func() error {
|
||||||
return formatter.Run(ctx)
|
return formatter.Run(ctx)
|
||||||
})
|
})
|
||||||
|
@ -180,7 +182,7 @@ func (f *Format) Run() error {
|
||||||
eg.Go(func() error {
|
eg.Go(func() error {
|
||||||
// pass paths to each formatter
|
// pass paths to each formatter
|
||||||
for path := range pathsCh {
|
for path := range pathsCh {
|
||||||
for _, formatter := range cfg.Formatters {
|
for _, formatter := range formatters {
|
||||||
if formatter.Wants(path) {
|
if formatter.Wants(path) {
|
||||||
formatter.Put(path)
|
formatter.Put(path)
|
||||||
}
|
}
|
||||||
|
@ -188,7 +190,7 @@ func (f *Format) Run() error {
|
||||||
}
|
}
|
||||||
|
|
||||||
// indicate no more paths for each formatter
|
// indicate no more paths for each formatter
|
||||||
for _, formatter := range cfg.Formatters {
|
for _, formatter := range formatters {
|
||||||
if formatter.Parent() != nil {
|
if formatter.Parent() != nil {
|
||||||
// this formatter is not a root, it will be closed by a parent
|
// this formatter is not a root, it will be closed by a parent
|
||||||
continue
|
continue
|
||||||
|
@ -197,7 +199,7 @@ func (f *Format) Run() error {
|
||||||
}
|
}
|
||||||
|
|
||||||
// await completion
|
// await completion
|
||||||
for _, formatter := range cfg.Formatters {
|
for _, formatter := range formatters {
|
||||||
formatter.AwaitCompletion()
|
formatter.AwaitCompletion()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -25,7 +25,7 @@ func TestAllowMissingFormatter(t *testing.T) {
|
||||||
configPath := tempDir + "/treefmt.toml"
|
configPath := tempDir + "/treefmt.toml"
|
||||||
|
|
||||||
test.WriteConfig(t, configPath, format.Config{
|
test.WriteConfig(t, configPath, format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"foo-fmt": {
|
"foo-fmt": {
|
||||||
Command: "foo-fmt",
|
Command: "foo-fmt",
|
||||||
},
|
},
|
||||||
|
@ -46,7 +46,7 @@ func TestSpecifyingFormatters(t *testing.T) {
|
||||||
configPath := tempDir + "/treefmt.toml"
|
configPath := tempDir + "/treefmt.toml"
|
||||||
|
|
||||||
test.WriteConfig(t, configPath, format.Config{
|
test.WriteConfig(t, configPath, format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"elm": {
|
"elm": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*.elm"},
|
Includes: []string{"*.elm"},
|
||||||
|
@ -95,7 +95,7 @@ func TestIncludesAndExcludes(t *testing.T) {
|
||||||
|
|
||||||
// test without any excludes
|
// test without any excludes
|
||||||
config := format.Config{
|
config := format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"echo": {
|
"echo": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*"},
|
Includes: []string{"*"},
|
||||||
|
@ -167,7 +167,7 @@ func TestCache(t *testing.T) {
|
||||||
|
|
||||||
// test without any excludes
|
// test without any excludes
|
||||||
config := format.Config{
|
config := format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"echo": {
|
"echo": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*"},
|
Includes: []string{"*"},
|
||||||
|
@ -202,7 +202,7 @@ func TestChangeWorkingDirectory(t *testing.T) {
|
||||||
|
|
||||||
// test without any excludes
|
// test without any excludes
|
||||||
config := format.Config{
|
config := format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"echo": {
|
"echo": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*"},
|
Includes: []string{"*"},
|
||||||
|
@ -227,7 +227,7 @@ func TestFailOnChange(t *testing.T) {
|
||||||
|
|
||||||
// test without any excludes
|
// test without any excludes
|
||||||
config := format.Config{
|
config := format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"echo": {
|
"echo": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*"},
|
Includes: []string{"*"},
|
||||||
|
@ -263,7 +263,7 @@ func TestBustCacheOnFormatterChange(t *testing.T) {
|
||||||
|
|
||||||
// start with 2 formatters
|
// start with 2 formatters
|
||||||
config := format.Config{
|
config := format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"python": {
|
"python": {
|
||||||
Command: "black",
|
Command: "black",
|
||||||
Includes: []string{"*.py"},
|
Includes: []string{"*.py"},
|
||||||
|
@ -307,7 +307,7 @@ func TestBustCacheOnFormatterChange(t *testing.T) {
|
||||||
as.Contains(string(out), "0 files changed")
|
as.Contains(string(out), "0 files changed")
|
||||||
|
|
||||||
// add go formatter
|
// add go formatter
|
||||||
config.Formatters["go"] = &format.Formatter{
|
config.Formatters["go"] = &format.FormatterConfig{
|
||||||
Command: "gofmt",
|
Command: "gofmt",
|
||||||
Options: []string{"-w"},
|
Options: []string{"-w"},
|
||||||
Includes: []string{"*.go"},
|
Includes: []string{"*.go"},
|
||||||
|
@ -413,7 +413,7 @@ func TestOrderingFormatters(t *testing.T) {
|
||||||
|
|
||||||
// missing child
|
// missing child
|
||||||
test.WriteConfig(t, configPath, format.Config{
|
test.WriteConfig(t, configPath, format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"hs-a": {
|
"hs-a": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*.hs"},
|
Includes: []string{"*.hs"},
|
||||||
|
@ -427,7 +427,7 @@ func TestOrderingFormatters(t *testing.T) {
|
||||||
|
|
||||||
// multiple roots
|
// multiple roots
|
||||||
test.WriteConfig(t, configPath, format.Config{
|
test.WriteConfig(t, configPath, format.Config{
|
||||||
Formatters: map[string]*format.Formatter{
|
Formatters: map[string]*format.FormatterConfig{
|
||||||
"hs-a": {
|
"hs-a": {
|
||||||
Command: "echo",
|
Command: "echo",
|
||||||
Includes: []string{"*.hs"},
|
Includes: []string{"*.hs"},
|
||||||
|
|
|
@ -8,7 +8,7 @@ type Config struct {
|
||||||
// Excludes is an optional list of glob patterns used to exclude certain files from all formatters.
|
// Excludes is an optional list of glob patterns used to exclude certain files from all formatters.
|
||||||
Excludes []string
|
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.
|
// ReadConfigFile reads from path and unmarshals toml into a Config instance.
|
||||||
|
|
|
@ -14,8 +14,7 @@ import (
|
||||||
// ErrFormatterNotFound is returned when the Command for a Formatter is not available.
|
// ErrFormatterNotFound is returned when the Command for a Formatter is not available.
|
||||||
var ErrFormatterNotFound = errors.New("formatter not found")
|
var ErrFormatterNotFound = errors.New("formatter not found")
|
||||||
|
|
||||||
// Formatter represents a command which should be applied to a filesystem.
|
type FormatterConfig struct {
|
||||||
type Formatter struct {
|
|
||||||
// Command is the command invoke when applying this Formatter.
|
// Command is the command invoke when applying this Formatter.
|
||||||
Command string
|
Command string
|
||||||
// Options are an optional list of args to be passed to Command.
|
// Options are an optional list of args to be passed to Command.
|
||||||
|
@ -26,11 +25,17 @@ type Formatter struct {
|
||||||
Excludes []string
|
Excludes []string
|
||||||
// Before is the name of another formatter which must process a path after this one
|
// Before is the name of another formatter which must process a path after this one
|
||||||
Before string
|
Before string
|
||||||
|
}
|
||||||
brianmcgee marked this conversation as resolved
Outdated
zimbatm
commented
Personal taste thing: because of how many public/private fields there are, I would split this into a FormatterConfig that only contains the config and serializes/deserializes, and then the Formatter that gets generated from the config. That way, how to interact with the structs in the rest of the code becomes clearer. I don't think the Before field should be mutated in the code for example. Personal taste thing: because of how many public/private fields there are, I would split this into a FormatterConfig that only contains the config and serializes/deserializes, and then the Formatter that gets generated from the config. That way, how to interact with the structs in the rest of the code becomes clearer. I don't think the Before field should be mutated in the code for example.
brianmcgee
commented
It was a fair point, I've separated things out in It was a fair point, I've separated things out in https://git.numtide.com/numtide/treefmt/commit/c54265517754f4d67684e5810c87cbba0644ae0d
|
|||||||
|
|
||||||
|
// Formatter represents a command which should be applied to a filesystem.
|
||||||
|
type Formatter struct {
|
||||||
|
name string
|
||||||
|
config *FormatterConfig
|
||||||
|
|
||||||
name string
|
|
||||||
log *log.Logger
|
log *log.Logger
|
||||||
executable string // path to the executable described by Command
|
executable string // path to the executable described by Command
|
||||||
|
|
||||||
|
before string
|
||||||
parent *Formatter
|
parent *Formatter
|
||||||
child *Formatter
|
child *Formatter
|
||||||
|
|
||||||
|
@ -49,24 +54,35 @@ type Formatter struct {
|
||||||
batchSize int
|
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
|
// Executable returns the path to the executable defined by Command
|
||||||
func (f *Formatter) Executable() string {
|
func (f *Formatter) Executable() string {
|
||||||
return f.executable
|
return f.executable
|
||||||
}
|
}
|
||||||
|
|
||||||
// Init is used to initialise internal state before this Formatter is ready to accept paths.
|
// NewFormatter is used to create a new Formatter.
|
||||||
func (f *Formatter) Init(name string, globalExcludes []glob.Glob) error {
|
func NewFormatter(name string, config *FormatterConfig, globalExcludes []glob.Glob) (*Formatter, error) {
|
||||||
var err error
|
var err error
|
||||||
|
|
||||||
|
f := Formatter{}
|
||||||
// capture the name from the config file
|
// capture the name from the config file
|
||||||
f.name = name
|
f.name = name
|
||||||
|
f.config = config
|
||||||
|
f.before = config.Before
|
||||||
|
|
||||||
// test if the formatter is available
|
// test if the formatter is available
|
||||||
executable, err := exec.LookPath(f.Command)
|
executable, err := exec.LookPath(config.Command)
|
||||||
if errors.Is(err, exec.ErrNotFound) {
|
if errors.Is(err, exec.ErrNotFound) {
|
||||||
return ErrFormatterNotFound
|
return nil, ErrFormatterNotFound
|
||||||
} else if err != nil {
|
} else if err != nil {
|
||||||
return err
|
return nil, err
|
||||||
}
|
}
|
||||||
f.executable = executable
|
f.executable = executable
|
||||||
|
|
||||||
|
@ -77,18 +93,18 @@ func (f *Formatter) Init(name string, globalExcludes []glob.Glob) error {
|
||||||
f.inboxCh = make(chan string, f.batchSize)
|
f.inboxCh = make(chan string, f.batchSize)
|
||||||
f.completedCh = make(chan interface{}, 1)
|
f.completedCh = make(chan interface{}, 1)
|
||||||
|
|
||||||
f.includes, err = CompileGlobs(f.Includes)
|
f.includes, err = CompileGlobs(config.Includes)
|
||||||
if err != nil {
|
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 {
|
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...)
|
f.excludes = append(f.excludes, globalExcludes...)
|
||||||
|
|
||||||
return nil
|
return &f, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
func (f *Formatter) SetParent(formatter *Formatter) {
|
func (f *Formatter) SetParent(formatter *Formatter) {
|
||||||
|
@ -177,7 +193,7 @@ func (f *Formatter) apply(ctx context.Context) error {
|
||||||
}
|
}
|
||||||
|
|
||||||
// construct args, starting with config
|
// construct args, starting with config
|
||||||
args := f.Options
|
args := f.config.Options
|
||||||
|
|
||||||
// append each file path
|
// append each file path
|
||||||
for _, path := range f.batch {
|
for _, path := range f.batch {
|
||||||
|
@ -186,7 +202,7 @@ func (f *Formatter) apply(ctx context.Context) error {
|
||||||
|
|
||||||
// execute
|
// execute
|
||||||
start := time.Now()
|
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 {
|
if out, err := cmd.CombinedOutput(); err != nil {
|
||||||
f.log.Debugf("\n%v", string(out))
|
f.log.Debugf("\n%v", string(out))
|
||||||
|
|
Reference in New Issue
Block a user
formatter.Before is there 2 times
If you look at the string you can see why it's listed twice