From a6de5739716b3d9458b61d3f304268890aea6035 Mon Sep 17 00:00:00 2001 From: Jeffrey Faer Date: Mon, 8 Apr 2024 08:12:43 -0600 Subject: [PATCH] feat: Allow default options to be customzied with a flag. --- cmd/cmd.go | 43 ++++++++-- keepsorted/block.go | 4 +- keepsorted/keep_sorted.go | 4 +- keepsorted/keep_sorted_test.go | 150 +++++++++++++++++++++++---------- keepsorted/options.go | 89 ++++++++++++++++--- 5 files changed, 222 insertions(+), 68 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index 949bf52..e9fa308 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -30,9 +30,10 @@ import ( ) type Config struct { - id string - operation operation - modifiedLines []keepsorted.LineRange + id string + defaultOptions keepsorted.BlockOptions + operation operation + modifiedLines []keepsorted.LineRange } func (c *Config) FromFlags(fs *flag.FlagSet) { @@ -45,6 +46,9 @@ func (c *Config) FromFlags(fs *flag.FlagSet) { panic(err) } + c.defaultOptions = keepsorted.DefaultBlockOptions() + fs.Var(&blockOptionsFlag{&c.defaultOptions}, "default-options", "The options keep-sorted will use to sort. Per-block overrides apply on top of these options. Note: list options like prefix_order are not merged with per-block overrides. They are completely overridden.") + of := &operationFlag{op: &c.operation} if err := of.Set("fix"); err != nil { panic(err) @@ -54,6 +58,27 @@ func (c *Config) FromFlags(fs *flag.FlagSet) { fs.Var(&lineRangeFlag{lineRanges: &c.modifiedLines}, "lines", "Line ranges of the form \"start:end\". Only processes keep-sorted blocks that overlap with the given line ranges. Can only be used when fixing a single file.") } +type blockOptionsFlag struct { + opts *keepsorted.BlockOptions +} + +func (f *blockOptionsFlag) String() string { + return f.opts.String() +} + +func (f *blockOptionsFlag) Set(val string) error { + opts, err := keepsorted.ParseBlockOptions(val) + if err != nil { + return err + } + *f.opts = opts + return nil +} + +func (f *blockOptionsFlag) Type() string { + return "options" +} + var ( operations = map[string]operation{ "lint": lint, @@ -67,7 +92,7 @@ func knownModes() []string { return ms } -type operation func(id string, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) +type operation func(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) type operationFlag struct { op *operation @@ -185,16 +210,16 @@ func Run(c *Config, files []string) (ok bool, err error) { return false, errors.New("cannot specify modifiedLines with more than one file") } - return c.operation(c.id, files, c.modifiedLines) + return c.operation(keepsorted.New(c.id, c.defaultOptions), files, c.modifiedLines) } -func fix(id string, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { +func fix(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { for _, fn := range filenames { contents, err := read(fn) if err != nil { return false, err } - if want, alreadyFixed := keepsorted.New(id).Fix(contents, modifiedLines); fn == stdin || !alreadyFixed { + if want, alreadyFixed := fixer.Fix(contents, modifiedLines); fn == stdin || !alreadyFixed { if err := write(fn, want); err != nil { return false, err } @@ -203,14 +228,14 @@ func fix(id string, filenames []string, modifiedLines []keepsorted.LineRange) (o return true, nil } -func lint(id string, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { +func lint(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { var fs []*keepsorted.Finding for _, fn := range filenames { contents, err := read(fn) if err != nil { return false, err } - fs = append(fs, keepsorted.New(id).Findings(fn, contents, modifiedLines)...) + fs = append(fs, fixer.Findings(fn, contents, modifiedLines)...) } if len(fs) == 0 { diff --git a/keepsorted/block.go b/keepsorted/block.go index c292c85..b094241 100644 --- a/keepsorted/block.go +++ b/keepsorted/block.go @@ -109,7 +109,7 @@ func (f *Fixer) newBlocks(lines []string, offset int, include func(start, end in continue } - opts, err := f.parseBlockOptions(start.line, defaultOptions) + opts, err := parseBlockOptions(start.line, f.defaultOptions) if err != nil { // TODO(b/250608236): Is there a better way to surface this error? log.Err(fmt.Errorf("keep-sorted block at index %d had bad start directive: %w", start.index+offset, err)).Msg("") @@ -238,7 +238,7 @@ func (b block) sorted() (sorted []string, alreadySorted bool) { } groups := groupLines(lines, b.metadata) - log.Printf("%d groups for block at index %d are (options %#v)", len(groups), b.start, b.metadata.opts) + log.Printf("%d groups for block at index %d are (options %v)", len(groups), b.start, b.metadata.opts) for _, lg := range groups { log.Printf("%#v", lg) } diff --git a/keepsorted/keep_sorted.go b/keepsorted/keep_sorted.go index 4196d58..45b9a03 100644 --- a/keepsorted/keep_sorted.go +++ b/keepsorted/keep_sorted.go @@ -31,15 +31,17 @@ const ( type Fixer struct { ID string + defaultOptions blockOptions startDirective string endDirective string } // New creates a new fixer with the given string as its identifier. // By default, id is "keep-sorted" -func New(id string) *Fixer { +func New(id string, defaultOptions BlockOptions) *Fixer { return &Fixer{ ID: id, + defaultOptions: defaultOptions.opts, startDirective: id + " start", endDirective: id + " end", } diff --git a/keepsorted/keep_sorted_test.go b/keepsorted/keep_sorted_test.go index 307030f..e3be0bf 100644 --- a/keepsorted/keep_sorted_test.go +++ b/keepsorted/keep_sorted_test.go @@ -15,10 +15,13 @@ package keepsorted import ( + "fmt" + "reflect" "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/rs/zerolog" "github.com/rs/zerolog/log" ) @@ -31,20 +34,6 @@ func initZerolog(t testing.TB) { t.Cleanup(func() { log.Logger = oldLogger }) } -func defaultOptionsWithCommentMarker(marker string) blockOptions { - opts := defaultOptions - opts.StickyComments = true - opts.setCommentMarker(marker) - return opts -} - -func optionsWithCommentMarker(marker string) blockOptions { - var opts blockOptions - opts.StickyComments = true - opts.setCommentMarker(marker) - return opts -} - func defaultMetadataWith(opts blockOptions) blockMetadata { return blockMetadata{ startDirective: "keep-sorted-test start", @@ -53,6 +42,12 @@ func defaultMetadataWith(opts blockOptions) blockMetadata { } } +func defaultMetadataWithCommentMarker(marker string) blockMetadata { + var opts blockOptions + opts.setCommentMarker(marker) + return defaultMetadataWith(opts) +} + func TestFix(t *testing.T) { for _, tc := range []struct { name string @@ -179,7 +174,7 @@ foo } { t.Run(tc.name, func(t *testing.T) { initZerolog(t) - got, gotAlreadyFixed := New("keep-sorted-test").Fix(tc.in, nil) + got, gotAlreadyFixed := New("keep-sorted-test", BlockOptions{}).Fix(tc.in, nil) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("Fix diff (-want +got):\n%s", diff) } @@ -320,7 +315,7 @@ baz mod = append(mod, LineRange{l, l}) } } - got := New("keep-sorted-test").findings(filename, strings.Split(tc.in, "\n"), mod, tc.considerLintOption) + got := New("keep-sorted-test", BlockOptions{}).findings(filename, strings.Split(tc.in, "\n"), mod, tc.considerLintOption) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("Findings diff (-want +got):\n%s", diff) } @@ -360,7 +355,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 3, end: 7, lines: []string{ @@ -370,7 +365,7 @@ cat`, }, }, { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 9, end: 13, lines: []string{ @@ -397,7 +392,7 @@ dog wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 7, lines: []string{ @@ -435,7 +430,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 3, end: 7, lines: []string{ @@ -463,7 +458,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 1, end: 6, lines: []string{ @@ -496,7 +491,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 1, end: 13, lines: []string{ @@ -514,7 +509,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 9, lines: []string{ @@ -574,7 +569,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 1, end: 34, lines: []string{ @@ -613,7 +608,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 30, lines: []string{ @@ -644,7 +639,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 9, end: 21, lines: []string{ @@ -662,7 +657,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 13, end: 17, lines: []string{ @@ -674,7 +669,7 @@ i }, }, { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 22, end: 26, lines: []string{ @@ -688,7 +683,7 @@ i }, }, { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 35, end: 39, lines: []string{ @@ -714,7 +709,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 7, lines: []string{"2"}, @@ -732,7 +727,7 @@ i tc.include = func(start, end int) bool { return true } } - gotBlocks, gotIncompleteBlocks := New("keep-sorted-test").newBlocks(strings.Split(tc.in, "\n"), 0, tc.include) + gotBlocks, gotIncompleteBlocks := New("keep-sorted-test", BlockOptions{}).newBlocks(strings.Split(tc.in, "\n"), 0, tc.include) if diff := cmp.Diff(tc.wantBlocks, gotBlocks, cmp.AllowUnexported(block{}, blockMetadata{}, blockOptions{})); diff != "" { t.Errorf("blocks diff (-want +got):\n%s", diff) } @@ -861,7 +856,13 @@ func TestLineSorting(t *testing.T) { { name: "CommentOnlyBlock", - opts: optionsWithCommentMarker("//"), + opts: func() blockOptions { + opts := blockOptions{ + StickyComments: true, + } + opts.setCommentMarker("//") + return opts + }(), in: []string{ "2", "1", @@ -919,8 +920,11 @@ func TestLineSorting(t *testing.T) { name: "RemoveDuplicates_ConsidersComments", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.RemoveDuplicates = true + opts := blockOptions{ + RemoveDuplicates: true, + StickyComments: true, + } + opts.setCommentMarker("//") return opts }(), in: []string{ @@ -1180,7 +1184,13 @@ func TestLineGrouping(t *testing.T) { }, { name: "StickyComments", - opts: optionsWithCommentMarker("//"), + opts: func() blockOptions { + opts := blockOptions{ + StickyComments: true, + } + opts.setCommentMarker("//") + return opts + }(), want: []lineGroup{ { @@ -1203,7 +1213,13 @@ func TestLineGrouping(t *testing.T) { }, { name: "CommentOnlyGroup", - opts: optionsWithCommentMarker("//"), + opts: func() blockOptions { + opts := blockOptions{ + StickyComments: true, + } + opts.setCommentMarker("//") + return opts + }(), want: []lineGroup{ { @@ -1291,8 +1307,11 @@ func TestLineGrouping(t *testing.T) { { name: "Group_NestedKeepSortedBlocksWithoutAnyIndentation", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.Group = true + opts := blockOptions{ + Group: true, + StickyComments: true, + } + opts.setCommentMarker("//") return opts }(), @@ -1425,8 +1444,10 @@ func TestLineGrouping(t *testing.T) { { name: "Block_IgnoresSpecialCharactersWithinFullLineComments", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.Block = true + opts := blockOptions{ + Block: true, + } + opts.setCommentMarker("//") return opts }(), @@ -1450,8 +1471,10 @@ func TestLineGrouping(t *testing.T) { { name: "Block_IgnoresSpecialCharactersWithinTrailingComments", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.Block = true + opts := blockOptions{ + Block: true, + } + opts.setCommentMarker("//") return opts }(), @@ -1628,7 +1651,7 @@ func TestBlockOptions(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { initZerolog(t) - got, err := New("keep-sorted-test").parseBlockOptions(tc.in, tc.defaultOptions) + got, err := parseBlockOptions(tc.in, tc.defaultOptions) if err != nil { if tc.wantErr == "" { t.Errorf("parseBlockOptions(%q) = %v", tc.in, err) @@ -1639,6 +1662,47 @@ func TestBlockOptions(t *testing.T) { if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(blockOptions{})); diff != "" { t.Errorf("parseBlockOptions(%q) mismatch (-want +got):\n%s", tc.in, diff) } + + _ = got.String() // Make sure this doesn't panic. }) } } + +func TestBlockOptions_ClonesDefaultOptions(t *testing.T) { + defaults := blockOptions{ + StickyPrefixes: map[string]bool{}, + } + _, err := parseBlockOptions("sticky_prefixes=//", defaults) + if err != nil { + t.Errorf("parseBlockOptions() = _, %v", err) + } + if diff := cmp.Diff(blockOptions{}, defaults, cmp.AllowUnexported(blockOptions{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("defaults appear to have been modified (-want +got):\n%s", diff) + } +} + +func TestBlockOptions_ClonesDefaultOptions_Reflection(t *testing.T) { + defaults := blockOptions{} + defaultOpts := reflect.ValueOf(&defaults).Elem() + var s []string + for i := 0; i < defaultOpts.NumField(); i++ { + val := defaultOpts.Field(i) + switch val.Kind() { + case reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128, reflect.String: + continue + case reflect.Slice: + val.Set(reflect.MakeSlice(val.Type(), 0, 0)) + s = append(s, fmt.Sprintf("%s=a,b,c", key(defaultOpts.Type().Field(i)))) + case reflect.Map: + val.Set(reflect.MakeMap(val.Type())) + s = append(s, fmt.Sprintf("%s=a,b,c", key(defaultOpts.Type().Field(i)))) + default: + t.Fatalf("unhandled type in blockOptions: %v", val.Type()) + } + + } + _, _ = parseBlockOptions(strings.Join(s, " "), defaults) + if diff := cmp.Diff(blockOptions{}, defaults, cmp.AllowUnexported(blockOptions{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("defaults appear to have been modified (-want +got):\n%s", diff) + } +} diff --git a/keepsorted/options.go b/keepsorted/options.go index 1e5a72c..a2ce6f0 100644 --- a/keepsorted/options.go +++ b/keepsorted/options.go @@ -25,6 +25,8 @@ import ( "strconv" "strings" "unicode" + + "golang.org/x/exp/maps" ) var ( @@ -34,8 +36,32 @@ var ( "no": false, "false": false, } + boolString = map[bool]string{ + true: "yes", + false: "no", + } ) +type BlockOptions struct { + opts blockOptions +} + +func DefaultBlockOptions() BlockOptions { + return BlockOptions{defaultOptions} +} + +func ParseBlockOptions(startLine string) (BlockOptions, error) { + opts, err := parseBlockOptions(startLine, blockOptions{}) + if err != nil { + return BlockOptions{}, err + } + return BlockOptions{opts}, nil +} + +func (opts BlockOptions) String() string { + return opts.opts.String() +} + // blockOptions enable/disable extra features that control how a block of lines is sorted. // // Currently, only four types are supported: @@ -97,12 +123,9 @@ var defaultOptions = blockOptions{ StickyPrefixes: nil, // Will be populated with the comment marker of the start directive. CaseSensitive: true, RemoveDuplicates: true, - // If we ever add a default map or slice or other reference type to this - // struct, we'll want to make sure we're doing a deep copy in - // parseBlockOptions. } -func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (blockOptions, error) { +func parseBlockOptions(startLine string, defaults blockOptions) (blockOptions, error) { ret := defaults opts := reflect.ValueOf(&ret) var errs error @@ -133,7 +156,7 @@ func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (bloc ret.GroupPrefixes = nil } - if cm := f.guessCommentMarker(startLine); cm != "" { + if cm := guessCommentMarker(startLine); cm != "" { ret.setCommentMarker(cm) } if len(ret.IgnorePrefixes) > 1 { @@ -147,11 +170,7 @@ func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (bloc var errOptionNotSet = errors.New("not set in start directive") func parseBlockOption(f reflect.StructField, startLine string) (reflect.Value, error) { - key := strings.ToLower(f.Name) - if k, ok := f.Tag.Lookup("key"); ok { - key = k - } - + key := key(f) regex := regexp.MustCompile(fmt.Sprintf(`(^|\s)%s=(?P[^ ]+?)($|\s)`, regexp.QuoteMeta(key))) if m := regex.FindStringSubmatchIndex(startLine); m != nil { val := string(regex.ExpandString(nil, "${value}", startLine, m)) @@ -160,9 +179,17 @@ func parseBlockOption(f reflect.StructField, startLine string) (reflect.Value, e return reflect.Zero(f.Type), fmt.Errorf("option %q: %w", key, errOptionNotSet) } +func key(f reflect.StructField) string { + key := strings.ToLower(f.Name) + if k, ok := f.Tag.Lookup("key"); ok { + key = k + } + return key +} + func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { switch f.Type { - case reflect.TypeOf(true): + case reflect.TypeOf(bool(false)): b, ok := boolValues[val] if !ok { return reflect.Zero(f.Type), fmt.Errorf("option %q has unknown value %q", key, val) @@ -186,7 +213,7 @@ func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { m[s] = true } return reflect.ValueOf(m), nil - case reflect.TypeOf(0): + case reflect.TypeOf(int(0)): if val == "" { return reflect.Zero(f.Type), nil } @@ -201,7 +228,24 @@ func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { panic(fmt.Errorf("unsupported blockOptions type: %v", f.Type)) } -func (f *Fixer) guessCommentMarker(startLine string) string { +func formatValue(val reflect.Value) string { + switch val.Type() { + case reflect.TypeOf(bool(false)): + return boolString[val.Bool()] + case reflect.TypeOf([]string{}): + return strings.Join(val.Interface().([]string), ",") + case reflect.TypeOf(map[string]bool{}): + keys := maps.Keys(val.Interface().(map[string]bool)) + slices.Sort(keys) + return strings.Join(keys, ",") + case reflect.TypeOf(int(0)): + return strconv.Itoa(int(val.Int())) + } + + panic(fmt.Errorf("unsupported blockOptions type: %v", val.Type())) +} + +func guessCommentMarker(startLine string) string { startLine = strings.TrimSpace(startLine) if strings.HasPrefix(startLine, "//") { return "//" @@ -229,6 +273,25 @@ func (opts *blockOptions) setCommentMarker(marker string) { } } +func (opts blockOptions) String() string { + var s []string + val := reflect.ValueOf(opts) + for i := 0; i < val.NumField(); i++ { + field := val.Type().Field(i) + if !field.IsExported() { + continue + } + + fieldVal := val.FieldByIndex(field.Index) + if fieldVal.IsZero() { + continue + } + s = append(s, fmt.Sprintf("%s=%s", key(field), formatValue(fieldVal))) + } + + return strings.Join(s, " ") +} + // hasPrefix determines if s has one of the prefixes. func hasPrefix(s string, prefixes map[string]bool) bool { if len(prefixes) == 0 {