From d82c7ab69f693d91aadd27a8b928327dddbbb32f Mon Sep 17 00:00:00 2001 From: braydonk Date: Thu, 31 Oct 2024 23:07:12 +0000 Subject: [PATCH] command: correctly propogate line_ending setting The line_ending setting from the formatter was accidentally being ignored in favour of only checking the top level. This PR refactors the code while taking this edge case into account. --- command/command.go | 56 ++++++++++++++---------------- command/command_test.go | 32 +++++++++++++++++ formatter.go | 3 ++ formatters/basic/formatter.go | 2 ++ formatters/basic/formatter_test.go | 25 +++++++++++++ 5 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 command/command_test.go diff --git a/command/command.go b/command/command.go index 0813072..3f75c40 100644 --- a/command/command.go +++ b/command/command.go @@ -60,35 +60,9 @@ type Command struct { } func (c *Command) Run() error { - var formatter yamlfmt.Formatter - if c.Config.FormatterConfig == nil { - factory, err := c.Registry.GetDefaultFactory() - if err != nil { - return err - } - formatter, err = factory.NewFormatter(nil) - if err != nil { - return err - } - } else { - var ( - factory yamlfmt.Factory - err error - ) - if c.Config.FormatterConfig.Type == "" { - factory, err = c.Registry.GetDefaultFactory() - } else { - factory, err = c.Registry.GetFactory(c.Config.FormatterConfig.Type) - } - if err != nil { - return err - } - - c.Config.FormatterConfig.FormatterSettings["line_ending"] = c.Config.LineEnding - formatter, err = factory.NewFormatter(c.Config.FormatterConfig.FormatterSettings) - if err != nil { - return err - } + formatter, err := c.getFormatter() + if err != nil { + return err } lineSepChar, err := c.Config.LineEnding.Separator() @@ -191,6 +165,30 @@ func (c *Command) Run() error { return nil } +func (c *Command) getFormatter() (yamlfmt.Formatter, error) { + var factoryType string + + // In the existing codepaths, this value is always set. But + // it's a habit of mine to check anything that can possibly be nil + // if I remember that to be the case. :) + if c.Config.FormatterConfig != nil { + factoryType = c.Config.FormatterConfig.Type + + // The line ending set within the formatter settings takes precedence over setting + // it from the top level config. If it's not set in formatter settings, then + // we use the value from the top level. + if _, ok := c.Config.FormatterConfig.FormatterSettings["line_ending"]; !ok { + c.Config.FormatterConfig.FormatterSettings["line_ending"] = c.Config.LineEnding + } + } + + factory, err := c.Registry.GetFactory(factoryType) + if err != nil { + return nil, err + } + return factory.NewFormatter(c.Config.FormatterConfig.FormatterSettings) +} + func (c *Command) collectPaths() ([]string, error) { collector := c.makePathCollector() return collector.CollectPaths() diff --git a/command/command_test.go b/command/command_test.go new file mode 100644 index 0000000..8262a9c --- /dev/null +++ b/command/command_test.go @@ -0,0 +1,32 @@ +package command + +import ( + "testing" + + "github.com/google/yamlfmt" + "github.com/google/yamlfmt/formatters/basic" + "github.com/google/yamlfmt/internal/assert" +) + +// This test asserts the proper behaviour for `line_ending` settings specified +// in formatter settings overriding the global configuration. +func TestLineEndingFormatterVsGloabl(t *testing.T) { + c := &Command{ + Config: &Config{ + LineEnding: "lf", + FormatterConfig: &FormatterConfig{ + FormatterSettings: map[string]any{ + "line_ending": yamlfmt.LineBreakStyleLF, + }, + }, + }, + Registry: yamlfmt.NewFormatterRegistry(&basic.BasicFormatterFactory{}), + } + + f, err := c.getFormatter() + assert.NilErr(t, err) + configMap, err := f.ConfigMap() + assert.NilErr(t, err) + formatterLineEnding := configMap["line_ending"].(yamlfmt.LineBreakStyle) + assert.Assert(t, formatterLineEnding == yamlfmt.LineBreakStyleLF, "expected formatter's line ending to be lf") +} diff --git a/formatter.go b/formatter.go index 2a17475..ce432ee 100644 --- a/formatter.go +++ b/formatter.go @@ -46,6 +46,9 @@ func (r *Registry) Add(f Factory) { } func (r *Registry) GetFactory(fType string) (Factory, error) { + if fType == "" { + return r.GetDefaultFactory() + } factory, ok := r.registry[fType] if !ok { return nil, fmt.Errorf("no formatter registered with type \"%s\"", fType) diff --git a/formatters/basic/formatter.go b/formatters/basic/formatter.go index 377409b..442e758 100644 --- a/formatters/basic/formatter.go +++ b/formatters/basic/formatter.go @@ -17,6 +17,7 @@ package basic import ( "bytes" "errors" + "fmt" "io" "github.com/braydonk/yaml" @@ -102,6 +103,7 @@ func (f *BasicFormatter) getNewEncoder(buf *bytes.Buffer) *yaml.Encoder { e.SetWidth(f.Config.LineLength) } + fmt.Println(f.Config.LineEnding) if f.Config.LineEnding == yamlfmt.LineBreakStyleCRLF { e.SetLineBreakStyle(yaml.LineBreakStyleCRLF) } diff --git a/formatters/basic/formatter_test.go b/formatters/basic/formatter_test.go index ba01175..32ed0c4 100644 --- a/formatters/basic/formatter_test.go +++ b/formatters/basic/formatter_test.go @@ -15,6 +15,7 @@ package basic_test import ( + "runtime" "strings" "testing" @@ -351,3 +352,27 @@ b: 2 t.Fatalf("expected: '%s', got: '%s'", expectedYml, resultStr) } } + +func TestWindowsEOFNewlineCRLFBug(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("this test is Windows specific") + } + + config := basic.DefaultConfig() + config.RetainLineBreaks = false + config.EOFNewline = true + config.LineEnding = yamlfmt.LineBreakStyleLF + f := newFormatter(config) + + yml := `a: 1 +b: 2` + + result, err := f.Format([]byte(yml)) + if err != nil { + t.Fatalf("expected formatting to pass, returned error: %v", err) + } + resultStr := string(result) + if strings.Contains(resultStr, "\r") { + t.Fatalf("expected: no CRLF, found it") + } +}