feat: Allow default options to be customzied with a flag. (#35)
diff --git a/cmd/cmd.go b/cmd/cmd.go
index 949bf52..75fa451 100644
--- a/cmd/cmd.go
+++ b/cmd/cmd.go
@@ -30,9 +30,10 @@
)
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,13 +46,37 @@
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)
}
fs.Var(of, "mode", fmt.Sprintf("Determines what mode to run this tool in. One of %q", knownModes()))
- 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.")
+ 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. This flag can either be a comma-separated list of line ranges, or it can be specified multiple times on the command line to specify multiple line ranges.")
+}
+
+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 (
@@ -67,7 +92,7 @@
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
@@ -83,6 +108,7 @@
if op == nil {
return fmt.Errorf("unknown mode %q. Valid modes: %q", val, knownModes())
}
+ f.s = val
*f.op = op
return nil
}
@@ -116,7 +142,7 @@
}
func (f *lineRangeFlag) Type() string {
- return "line ranges"
+ return "line_ranges"
}
func (f *lineRangeFlag) Append(val string) error {
@@ -185,16 +211,16 @@
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 +229,14 @@
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 @@
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 @@
}
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 @@
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..0abcd04 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 @@
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 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 @@
} {
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 @@
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 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 3,
end: 7,
lines: []string{
@@ -370,7 +365,7 @@
},
},
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 9,
end: 13,
lines: []string{
@@ -397,7 +392,7 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 5,
end: 7,
lines: []string{
@@ -435,7 +430,7 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 3,
end: 7,
lines: []string{
@@ -463,7 +458,7 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 1,
end: 6,
lines: []string{
@@ -496,7 +491,7 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 1,
end: 13,
lines: []string{
@@ -514,7 +509,7 @@
},
nestedBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 5,
end: 9,
lines: []string{
@@ -574,7 +569,7 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 1,
end: 34,
lines: []string{
@@ -613,7 +608,7 @@
},
nestedBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 5,
end: 30,
lines: []string{
@@ -644,7 +639,7 @@
},
nestedBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 9,
end: 21,
lines: []string{
@@ -662,7 +657,7 @@
},
nestedBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 13,
end: 17,
lines: []string{
@@ -674,7 +669,7 @@
},
},
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 22,
end: 26,
lines: []string{
@@ -688,7 +683,7 @@
},
},
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 35,
end: 39,
lines: []string{
@@ -714,7 +709,7 @@
wantBlocks: []block{
{
- metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")),
+ metadata: defaultMetadataWithCommentMarker("//"),
start: 5,
end: 7,
lines: []string{"2"},
@@ -732,7 +727,7 @@
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 @@
{
name: "CommentOnlyBlock",
- opts: optionsWithCommentMarker("//"),
+ opts: func() blockOptions {
+ opts := blockOptions{
+ StickyComments: true,
+ }
+ opts.setCommentMarker("//")
+ return opts
+ }(),
in: []string{
"2",
"1",
@@ -919,8 +920,11 @@
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 @@
},
{
name: "StickyComments",
- opts: optionsWithCommentMarker("//"),
+ opts: func() blockOptions {
+ opts := blockOptions{
+ StickyComments: true,
+ }
+ opts.setCommentMarker("//")
+ return opts
+ }(),
want: []lineGroup{
{
@@ -1203,7 +1213,13 @@
},
{
name: "CommentOnlyGroup",
- opts: optionsWithCommentMarker("//"),
+ opts: func() blockOptions {
+ opts := blockOptions{
+ StickyComments: true,
+ }
+ opts.setCommentMarker("//")
+ return opts
+ }(),
want: []lineGroup{
{
@@ -1291,8 +1307,11 @@
{
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 @@
{
name: "Block_IgnoresSpecialCharactersWithinFullLineComments",
opts: func() blockOptions {
- opts := optionsWithCommentMarker("//")
- opts.Block = true
+ opts := blockOptions{
+ Block: true,
+ }
+ opts.setCommentMarker("//")
return opts
}(),
@@ -1450,8 +1471,10 @@
{
name: "Block_IgnoresSpecialCharactersWithinTrailingComments",
opts: func() blockOptions {
- opts := optionsWithCommentMarker("//")
- opts.Block = true
+ opts := blockOptions{
+ Block: true,
+ }
+ opts.setCommentMarker("//")
return opts
}(),
@@ -1628,7 +1651,7 @@
} {
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 @@
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.Errorf("Option %q has unhandled type: %v", key(defaultOpts.Type().Field(i)), 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 @@
"strconv"
"strings"
"unicode"
+
+ "golang.org/x/exp/maps"
)
var (
@@ -34,8 +36,32 @@
"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 @@
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 @@
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 @@
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<value>[^ ]+?)($|\s)`, regexp.QuoteMeta(key)))
if m := regex.FindStringSubmatchIndex(startLine); m != nil {
val := string(regex.ExpandString(nil, "${value}", startLine, m))
@@ -160,9 +179,17 @@
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 @@
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 @@
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) 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 {
